fix(launcher): address late review findings#23
Merged
Conversation
Declare semantic launch-action fallback bindings, avoid duplicating the app id in launcher runtime fallback paths, and align dependency-helper docs with required Node tooling. Co-authored-by: Codex <noreply@openai.com>
📝 WalkthroughWalkthroughThis PR updates the Linux launcher's runtime path selection logic, clarifies Fedora dependency installation docs, refactors the launch-action code generator to gate tray/warm-start/prompt-window behavior via persisted-setting helpers, and adds a unit test ensuring patcher output remains syntactically valid. ChangesLinux Launcher Runtime and Launch-Action Configuration
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19d271e472
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR addresses follow-up review findings from the earlier upstream-sync work by tightening Linux launcher behavior and ensuring the Linux window UI patcher remains valid under strict-mode module execution, while also updating the build/run documentation to match the dependency helper’s actual Node.js/npm requirements.
Changes:
- Fix
scripts/patch-linux-window-ui.jssemantic launch-action patch generation to declare fallback bindings withlet(strict-mode safe). - Adjust
launcher/start.sh.templateso the launch-action socket path doesn’t redundantly append the app id whenXDG_RUNTIME_DIRis unset. - Update Fedora dependency-helper documentation to reflect that the helper verifies/installs the required Node.js/npm toolchain.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/patch-linux-window-ui.js | Declares semantic launch-action patch bindings with let to avoid strict-mode failures. |
| launcher/start.sh.template | Refines runtime/socket directory selection to avoid duplicating the app id when falling back from XDG_RUNTIME_DIR. |
| docs/usage/build-and-run.md | Aligns Fedora instructions with the dependency helper’s enforced Node.js/npm toolchain behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/patch-linux-window-ui.js`:
- Line 1594: The IIFE currently encloses helper functions (notably
codexLinuxIsTrayEnabled and codexLinuxPrewarmHotkeyWindow) so downstream patches
(applyLinuxTrayPatch, applyLinuxHotkeyWindowPrewarmPatch) that reference them
will throw ReferenceError; fix by moving those helpers out of the IIFE or
exposing them on a shared namespace object (e.g., attach codexLinuxIsTrayEnabled
and codexLinuxPrewarmHotkeyWindow to a global/shared object used by other
patches) so they are accessible across patches, and update any internal
references inside the IIFE to use the shared namespace if you choose exposure.
In `@scripts/patch-linux-window-ui.test.js`:
- Around line 422-432: The test currently only runs
applyLinuxLaunchActionArgsPatch in isolation; update it to feed the patched
output through the downstream launch-action-related patch functions used in the
real pipeline (i.e. after applyPatchTwice(applyLinuxLaunchActionArgsPatch,
source) also call the subsequent launch-action patches such as
applyLinuxLaunchActionPatch and applyLinuxLaunchActionHelpersPatch or whatever
downstream functions the repo uses), then assert the final output (after those
passes) does not contain external helper references (e.g., /,let
codexLinuxGetSetting/) and that new Function(finalPatched) does not throw; keep
references to applyLinuxLaunchActionArgsPatch and applyPatchTwice to locate the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cba02af7-eb3c-4a15-ac63-0345010dfc05
📒 Files selected for processing (2)
scripts/patch-linux-window-ui.jsscripts/patch-linux-window-ui.test.js
Prefix semantic launch-action fallback declarations with a no-op expression so comma-expression insertion points stay parseable while downstream patch passes can still read the shared helpers. Co-authored-by: Codex <noreply@openai.com>
nisavid
force-pushed
the
codex/fix-pr-22-late-review
branch
from
747d9e6 to
7b18a20
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
XDG_RUNTIME_DIRis unset.Verification
git diff --checkpassed.env -u 'BASH_FUNC_ml%%' -u 'BASH_FUNC_module%%' bash -n launcher/start.sh.template scripts/install-deps.sh tests/scripts_smoke.shpassed.node --check scripts/patch-linux-window-ui.jspassed.node --test scripts/patch-linux-window-ui.test.jspassed: 62 tests.env -u 'BASH_FUNC_ml%%' -u 'BASH_FUNC_module%%' bash tests/scripts_smoke.shpassed.stat -c '%n %Y %y' Codex.dmganddate -u '+%s %Y-%m-%dT%H:%M:%SZ'showedCodex.dmg 1778141935 2026-05-07 04:18:55.874873464 -0400and1778147882 2026-05-07T09:58:02Z, so the cached DMG was refreshed within 24 hours before the app/package gate.env -u 'BASH_FUNC_ml%%' -u 'BASH_FUNC_module%%' ./install.sh Codex.dmgpassed after rerunning outside the sandbox because sandboxed npm cache writes hitEROFS.env -u 'BASH_FUNC_ml%%' -u 'BASH_FUNC_module%%' ./scripts/build-pacman.shpassed and produceddist/codex-app-26.429.61741-1-x86_64.pkg.tar.zst.pacman -Qip dist/codex-app-26.429.61741-1-x86_64.pkg.tar.zstconfirmed packagecodex-appversion26.429.61741-1.bsdtar -tf dist/codex-app-26.429.61741-1-x86_64.pkg.tar.zst | rg 'opt/codex-app/start.sh$|usr/lib/codex-app/update-builder/scripts/patch-linux-window-ui.js$'confirmed the package containsopt/codex-app/start.shandusr/lib/codex-app/update-builder/scripts/patch-linux-window-ui.js.Current Status
7b18a20b3e52c14f75385ae27aa240076203c11d.Summary by CodeRabbit
Documentation
Bug Fixes
Tests
Greptile Summary
This PR fixes three late review findings from merged PR #22: a duplicated app-ID path segment in the launcher when
XDG_RUNTIME_DIRis unset, a,letsyntax error in the JS patch injector when the injection site is preceded by a comma expression, and a doc wording imprecision in the Fedora build guide.launcher/start.sh.template: Replaces${XDG_RUNTIME_DIR:-$APP_STATE_DIR}/$CODEX_LINUX_APP_ID(which appended the app ID twice underAPP_STATE_DIR) with an explicitif/elsethat falls back to$APP_STATE_DIRunchanged; fix applied both at the top level and inrefresh_runtime_paths.scripts/patch-linux-window-ui.js: Prefixes the injected block withvoid 0;letso a preceding comma expression (E&&ce(),<injected>) terminates asE&&ce(),void 0;before theletdeclarations, preventing a parse error; a new regression test verifies idempotency and syntactic validity vianew Function.docs/usage/build-and-run.md: Clarifies that the dependency helper verifies or installs the Node.js/npm toolchain rather than just "optionally" touching system tooling.Confidence Score: 5/5
Safe to merge — all three fixes are narrow, well-tested, and address confirmed bugs without introducing new code paths.
The launcher path fix removes a mechanical duplication (app-ID appended twice) that would have produced a non-existent socket directory on systems without XDG_RUNTIME_DIR. The JS injector fix is verified both by the new regression test and by node --test passing 62 tests per the PR description. The doc change is purely textual.
No files require special attention.
Important Files Changed
void 0;letso that a preceding comma expression terminates cleanly before the let declarations begin, preventing a ,let syntax error.Reviews (3): Last reviewed commit: "fix(patcher): keep semantic fallback exp..." | Re-trigger Greptile