feat: add Windows open-target and terminal pwsh patch scripts#18
Open
keejkrej wants to merge 2 commits intoHaleclipse:masterfrom
Open
feat: add Windows open-target and terminal pwsh patch scripts#18keejkrej wants to merge 2 commits intoHaleclipse:masterfrom
keejkrej wants to merge 2 commits intoHaleclipse:masterfrom
Conversation
Reviewer's GuideAdds two Windows-only patch scripts that post-process the bundled Electron main.js to enable Windows editor/file-manager open-targets and prefer PowerShell/pwsh for terminal sessions, using idempotent markers and string-based bundle surgery. Sequence diagram for Windows terminal pwsh patch behaviorsequenceDiagram
actor User
participant AppMain
participant TerminalManager
participant node_pty
participant WindowsShell
User->>AppMain: Request new terminal session
AppMain->>TerminalManager: create(sessionOptions)
TerminalManager->>TerminalManager: resolveShell()
alt NonWindows
TerminalManager->>TerminalManager: Use SHELL or /bin/bash
else Windows
TerminalManager->>TerminalManager: Search pwsh paths
TerminalManager->>TerminalManager: where pwsh.exe
alt pwsh found
TerminalManager->>TerminalManager: shell = pwsh.exe
else pwsh not found
TerminalManager->>TerminalManager: where powershell.exe
alt powershell found
TerminalManager->>TerminalManager: shell = powershell.exe
else none found
TerminalManager->>TerminalManager: shell = WindowsPowerShell default path
end
end
end
TerminalManager->>TerminalManager: Normalize terminalCommand list
TerminalManager->>TerminalManager: If Windows and cmd/pwsh/powershell
TerminalManager->>TerminalManager: Replace command with resolved shell
TerminalManager->>node_pty: spawn(shell, args, env, cwd)
node_pty-->>TerminalManager: pty instance
TerminalManager-->>AppMain: sessionId
AppMain-->>User: Terminal attached running PowerShell/pwsh
Sequence diagram for Windows open-in-targets detection and launchsequenceDiagram
actor User
participant AppMain
participant OpenInTargets
participant FileSystem
participant TargetApp
User->>AppMain: Open file in external editor
AppMain->>OpenInTargets: kO() listAvailableTargets
alt macOS
OpenInTargets->>OpenInTargets: Use macOS target list mS
else Windows
OpenInTargets->>OpenInTargets: Use __codexOpenTargetsWin
end
OpenInTargets->>FileSystem: detect() for each target
FileSystem-->>OpenInTargets: Exists or not
OpenInTargets-->>AppMain: Available target ids
User->>AppMain: Choose target (e.g. vscode or finder)
AppMain->>OpenInTargets: Tce(targetId, filePath, location)
alt Unsupported platform
OpenInTargets-->>AppMain: Throw error "only supported on macOS and Windows"
else macOS or Windows
OpenInTargets->>OpenInTargets: Find target in __codexOpenTargets
alt Editor target (vscode, vscodeInsiders, cursor, windsurf)
OpenInTargets->>OpenInTargets: Build args with __codexOpenTargetWinCodeArgs
else File manager target (finder mapped to Explorer)
OpenInTargets->>OpenInTargets: Build args with __codexOpenTargetWinExplorerArgs
end
OpenInTargets->>TargetApp: Spawn process with exe path and args
TargetApp-->>User: File opened in chosen app
end
Flow diagram for Windows patch scripts applying bundle surgeryflowchart TD
A[Start patch script] --> B[locateBundle]
B --> C[Compute buildDir src/.vite/build]
C --> D[Read directory and match main*.js]
D --> E[Select main bundle file]
E --> F[Read bundle source]
F --> G{Patch type}
G --> H[Apply terminal pwsh patch]
G --> I[Apply open-in-targets win patch]
H --> J{Source contains __codex_terminal_pwsh_windows_patch?}
J -->|Yes| R[Exit without changes]
J -->|No| K[Replace resolveShell implementation]
K --> L[Replace async create function]
L --> M[Append __codex_terminal_pwsh_windows_patch marker]
I --> N{Source contains __codex_open_targets_win_patch?}
N -->|Yes| R
N -->|No| O[Inject __codexOpenTargetsIsWin flag]
O --> P[Replace target map and detection logic]
P --> Q[Update functions kO and Tce to use __codexOpenTargets]
Q --> S[Append __codex_open_targets_win_patch marker]
M --> T[Write patched bundle]
S --> T
R --> U[End]
T --> U
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Both patch scripts duplicate
locateBundle(including thebuildDirpath and bundle selection logic); consider extracting this into a shared helper to reduce the risk of the implementations diverging over time. - The string-based markers used for
replace/replaceFunction(e.g.,resolveShell(){return process.platform===...) look very brittle to minification or upstream refactors; if possible, tighten the matching (e.g., via surrounding context or regex with sanity checks) or centralize these marker constants so they’re easier to update when the bundle shape changes. - Both scripts assume
src/.vite/buildexists and will throw onreaddirSyncif it does not; if this is likely to run in environments without a built bundle, consider adding an explicit existence check or clearer error to distinguish missing build output from other I/O failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both patch scripts duplicate `locateBundle` (including the `buildDir` path and bundle selection logic); consider extracting this into a shared helper to reduce the risk of the implementations diverging over time.
- The string-based markers used for `replace`/`replaceFunction` (e.g., `resolveShell(){return process.platform===...`) look very brittle to minification or upstream refactors; if possible, tighten the matching (e.g., via surrounding context or regex with sanity checks) or centralize these marker constants so they’re easier to update when the bundle shape changes.
- Both scripts assume `src/.vite/build` exists and will throw on `readdirSync` if it does not; if this is likely to run in environments without a built bundle, consider adding an explicit existence check or clearer error to distinguish missing build output from other I/O failures.
## Individual Comments
### Comment 1
<location path="scripts/patch-terminal-pwsh-win.js" line_range="36-40" />
<code_context>
+function replaceFunction(source, startNeedle, replacementHeader) {
+ const start = source.indexOf(startNeedle);
+ if (start < 0) throw new Error(`Missing function marker: ${startNeedle}`);
+ const open = source.indexOf("{", start);
+ const close = findMatchingBrace(source, open);
+ const end = close + 1;
+ return {
</code_context>
<issue_to_address>
**suggestion:** Guard against missing function body braces to avoid confusing errors from findMatchingBrace.
If `source.indexOf("{", start)` returns `-1` (e.g. the minified function header changes), `findMatchingBrace` is called with `open = -1` and will scan from the beginning before throwing a generic "Failed to find matching brace." error. It would be clearer to check `open < 0` first and throw an error that explicitly mentions the missing `{` for this `startNeedle` instead of relying on `findMatchingBrace`’s generic failure.
```suggestion
function replaceFunction(source, startNeedle, replacementHeader) {
const start = source.indexOf(startNeedle);
if (start < 0) throw new Error(`Missing function marker: ${startNeedle}`);
const open = source.indexOf("{", start);
if (open < 0) {
throw new Error(
`Missing opening brace "{" for function marker: ${startNeedle}`,
);
}
const close = findMatchingBrace(source, open);
```
</issue_to_address>
### Comment 2
<location path="scripts/patch-open-in-targets-win.js" line_range="22" />
<code_context>
+ const winTargets = `function __codexOpenTargetWinFromEnv(t,...e){const n=process.env[t];return n?ne.join(n,...e):null}function __codexOpenTargetWinFind(...t){for(const e of t){const n=Array.isArray(e)?e:[e];for(const r of n){if(!r)continue;if(ve.existsSync(r))return r}}return null}function __codexOpenTargetWinWhere(t){try{const n=Dn.spawnSync("where",[t],{encoding:"utf8",timeout:1e3}),r=(n.stdout?.split(/\\r?\\n/).map(e=>e.trim()).filter(Boolean))||[];for(const e of r)if(__codexOpenTargetWinFind(e))return e}catch{}return null}function __codexOpenTargetWinCodeArgs(t,e){if(e&&typeof e.line=="number"){const n=typeof e.column=="number"?e.column:1;return["-g",t+":"+e.line+":"+n]}return["-g",t]}function __codexOpenTargetWinExplorerArgs(t){return["/select,",ne.normalize(t)]}const __codexOpenTargetsWin=[{id:"vscode",label:"VS Code",icon:"apps/vscode.png",detect:()=>__codexOpenTargetWinFind(__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Microsoft VS Code","bin","code.cmd"),__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Microsoft VS Code","Code.exe"),__codexOpenTargetWinFromEnv("ProgramFiles","Microsoft VS Code","bin","code.cmd"),__codexOpenTargetWinFromEnv("ProgramFiles","Microsoft VS Code","Code.exe"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Microsoft VS Code","bin","code.cmd"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Microsoft VS Code","Code.exe"),__codexOpenTargetWinWhere("code.cmd"),__codexOpenTargetWinWhere("code"),__codexOpenTargetWinWhere("Code.exe")),args:(t,e)=>__codexOpenTargetWinCodeArgs(t,e)},{id:"vscodeInsiders",label:"VS Code Insiders",icon:"apps/vscode-insiders.png",detect:()=>__codexOpenTargetWinFind(__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Microsoft VS Code Insiders","bin","code-insiders.cmd"),__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Microsoft VS Code Insiders","Code - Insiders.exe"),__codexOpenTargetWinFromEnv("ProgramFiles","Microsoft VS Code Insiders","bin","code-insiders.cmd"),__codexOpenTargetWinFromEnv("ProgramFiles","Microsoft VS Code Insiders","Code - Insiders.exe"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Microsoft VS Code Insiders","bin","code-insiders.cmd"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Microsoft VS Code Insiders","Code - Insiders.exe"),__codexOpenTargetWinWhere("code-insiders.cmd"),__codexOpenTargetWinWhere("code-insiders"),__codexOpenTargetWinWhere("Code - Insiders.exe")),args:(t,e)=>__codexOpenTargetWinCodeArgs(t,e)},{id:"cursor",label:"Cursor",icon:"apps/cursor.png",detect:()=>__codexOpenTargetWinFind(__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Cursor","Cursor.exe"),__codexOpenTargetWinFromEnv("ProgramFiles","Cursor","Cursor.exe"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Cursor","Cursor.exe"),__codexOpenTargetWinWhere("cursor"),__codexOpenTargetWinWhere("cursor.cmd"),__codexOpenTargetWinWhere("Cursor.exe")),args:(t,e)=>__codexOpenTargetWinCodeArgs(t,e)},{id:"windsurf",label:"Windsurf",icon:"apps/windsurf.png",detect:()=>__codexOpenTargetWinFind(__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Windsurf","Windsurf.exe"),__codexOpenTargetWinFromEnv("ProgramFiles","Windsurf","Windsurf.exe"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Windsurf","Windsurf.exe"),__codexOpenTargetWinWhere("windsurf"),__codexOpenTargetWinWhere("windsurf.cmd"),__codexOpenTargetWinWhere("Windsurf.exe")),args:(t,e)=>__codexOpenTargetWinCodeArgs(t,e)},{id:"finder",label:"File Manager",icon:"apps/finder.png",detect:()=>__codexOpenTargetWinFind(__codexOpenTargetWinWhere("explorer"),__codexOpenTargetWinWhere("explorer.exe"),__codexOpenTargetWinWhere("explorer.com"),"C:\\\\Windows\\\\explorer.exe"),args:t=>__codexOpenTargetWinExplorerArgs(t)}];let Lh=null;const __codexOpenTargets=Yi?mS:__codexOpenTargetsIsWin?__codexOpenTargetsWin:[],yce=__codexOpenTargets.map(({id:t,label:e,icon:n})=>({id:t,label:e,icon:n}));`;
</code_context>
<issue_to_address>
**issue (bug_risk):** n.stdout optional chaining chain can still throw when stdout is undefined.
In `__codexOpenTargetWinWhere`, `r` is computed as `(n.stdout?.split(/\r?\n/).map(e=>e.trim()).filter(Boolean)) || []`. When `stdout` is `undefined`, `n.stdout?.split(...)` becomes `undefined`, so calling `.map` on it will throw before the `|| []` applies. To avoid this, you can first default `stdout` to an array of lines, then map/filter, e.g.:
```ts
const lines = n.stdout?.split(/\r?\n/) ?? [];
const r = lines.map(e => e.trim()).filter(Boolean);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+36
to
+40
| function replaceFunction(source, startNeedle, replacementHeader) { | ||
| const start = source.indexOf(startNeedle); | ||
| if (start < 0) throw new Error(`Missing function marker: ${startNeedle}`); | ||
| const open = source.indexOf("{", start); | ||
| const close = findMatchingBrace(source, open); |
There was a problem hiding this comment.
suggestion: Guard against missing function body braces to avoid confusing errors from findMatchingBrace.
If source.indexOf("{", start) returns -1 (e.g. the minified function header changes), findMatchingBrace is called with open = -1 and will scan from the beginning before throwing a generic "Failed to find matching brace." error. It would be clearer to check open < 0 first and throw an error that explicitly mentions the missing { for this startNeedle instead of relying on findMatchingBrace’s generic failure.
Suggested change
| function replaceFunction(source, startNeedle, replacementHeader) { | |
| const start = source.indexOf(startNeedle); | |
| if (start < 0) throw new Error(`Missing function marker: ${startNeedle}`); | |
| const open = source.indexOf("{", start); | |
| const close = findMatchingBrace(source, open); | |
| function replaceFunction(source, startNeedle, replacementHeader) { | |
| const start = source.indexOf(startNeedle); | |
| if (start < 0) throw new Error(`Missing function marker: ${startNeedle}`); | |
| const open = source.indexOf("{", start); | |
| if (open < 0) { | |
| throw new Error( | |
| `Missing opening brace "{" for function marker: ${startNeedle}`, | |
| ); | |
| } | |
| const close = findMatchingBrace(source, open); |
| if (!source.includes(targetMapStart)) { | ||
| throw new Error("Unable to find open target map marker in bundle."); | ||
| } | ||
| const winTargets = `function __codexOpenTargetWinFromEnv(t,...e){const n=process.env[t];return n?ne.join(n,...e):null}function __codexOpenTargetWinFind(...t){for(const e of t){const n=Array.isArray(e)?e:[e];for(const r of n){if(!r)continue;if(ve.existsSync(r))return r}}return null}function __codexOpenTargetWinWhere(t){try{const n=Dn.spawnSync("where",[t],{encoding:"utf8",timeout:1e3}),r=(n.stdout?.split(/\\r?\\n/).map(e=>e.trim()).filter(Boolean))||[];for(const e of r)if(__codexOpenTargetWinFind(e))return e}catch{}return null}function __codexOpenTargetWinCodeArgs(t,e){if(e&&typeof e.line=="number"){const n=typeof e.column=="number"?e.column:1;return["-g",t+":"+e.line+":"+n]}return["-g",t]}function __codexOpenTargetWinExplorerArgs(t){return["/select,",ne.normalize(t)]}const __codexOpenTargetsWin=[{id:"vscode",label:"VS Code",icon:"apps/vscode.png",detect:()=>__codexOpenTargetWinFind(__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Microsoft VS Code","bin","code.cmd"),__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Microsoft VS Code","Code.exe"),__codexOpenTargetWinFromEnv("ProgramFiles","Microsoft VS Code","bin","code.cmd"),__codexOpenTargetWinFromEnv("ProgramFiles","Microsoft VS Code","Code.exe"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Microsoft VS Code","bin","code.cmd"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Microsoft VS Code","Code.exe"),__codexOpenTargetWinWhere("code.cmd"),__codexOpenTargetWinWhere("code"),__codexOpenTargetWinWhere("Code.exe")),args:(t,e)=>__codexOpenTargetWinCodeArgs(t,e)},{id:"vscodeInsiders",label:"VS Code Insiders",icon:"apps/vscode-insiders.png",detect:()=>__codexOpenTargetWinFind(__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Microsoft VS Code Insiders","bin","code-insiders.cmd"),__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Microsoft VS Code Insiders","Code - Insiders.exe"),__codexOpenTargetWinFromEnv("ProgramFiles","Microsoft VS Code Insiders","bin","code-insiders.cmd"),__codexOpenTargetWinFromEnv("ProgramFiles","Microsoft VS Code Insiders","Code - Insiders.exe"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Microsoft VS Code Insiders","bin","code-insiders.cmd"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Microsoft VS Code Insiders","Code - Insiders.exe"),__codexOpenTargetWinWhere("code-insiders.cmd"),__codexOpenTargetWinWhere("code-insiders"),__codexOpenTargetWinWhere("Code - Insiders.exe")),args:(t,e)=>__codexOpenTargetWinCodeArgs(t,e)},{id:"cursor",label:"Cursor",icon:"apps/cursor.png",detect:()=>__codexOpenTargetWinFind(__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Cursor","Cursor.exe"),__codexOpenTargetWinFromEnv("ProgramFiles","Cursor","Cursor.exe"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Cursor","Cursor.exe"),__codexOpenTargetWinWhere("cursor"),__codexOpenTargetWinWhere("cursor.cmd"),__codexOpenTargetWinWhere("Cursor.exe")),args:(t,e)=>__codexOpenTargetWinCodeArgs(t,e)},{id:"windsurf",label:"Windsurf",icon:"apps/windsurf.png",detect:()=>__codexOpenTargetWinFind(__codexOpenTargetWinFromEnv("LOCALAPPDATA","Programs","Windsurf","Windsurf.exe"),__codexOpenTargetWinFromEnv("ProgramFiles","Windsurf","Windsurf.exe"),__codexOpenTargetWinFromEnv("ProgramFiles(x86)","Windsurf","Windsurf.exe"),__codexOpenTargetWinWhere("windsurf"),__codexOpenTargetWinWhere("windsurf.cmd"),__codexOpenTargetWinWhere("Windsurf.exe")),args:(t,e)=>__codexOpenTargetWinCodeArgs(t,e)},{id:"finder",label:"File Manager",icon:"apps/finder.png",detect:()=>__codexOpenTargetWinFind(__codexOpenTargetWinWhere("explorer"),__codexOpenTargetWinWhere("explorer.exe"),__codexOpenTargetWinWhere("explorer.com"),"C:\\\\Windows\\\\explorer.exe"),args:t=>__codexOpenTargetWinExplorerArgs(t)}];let Lh=null;const __codexOpenTargets=Yi?mS:__codexOpenTargetsIsWin?__codexOpenTargetsWin:[],yce=__codexOpenTargets.map(({id:t,label:e,icon:n})=>({id:t,label:e,icon:n}));`; |
There was a problem hiding this comment.
issue (bug_risk): n.stdout optional chaining chain can still throw when stdout is undefined.
In __codexOpenTargetWinWhere, r is computed as (n.stdout?.split(/\r?\n/).map(e=>e.trim()).filter(Boolean)) || []. When stdout is undefined, n.stdout?.split(...) becomes undefined, so calling .map on it will throw before the || [] applies. To avoid this, you can first default stdout to an array of lines, then map/filter, e.g.:
const lines = n.stdout?.split(/\r?\n/) ?? [];
const r = lines.map(e => e.trim()).filter(Boolean);
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
1 participant
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
This PR adds two Windows-only patch scripts to enable open-target and terminal-shell fixes in the bundled Electron main process.
Changes
patch-open-in-targets-win.js
patch-terminal-pwsh-win.js
Summary by Sourcery
Add Windows-only patch scripts that modify the bundled Electron main process to support Windows editor targets and prefer PowerShell-based shells for terminals.
New Features:
Enhancements: