fix: fix broken init site on Windows #1316
Conversation
|
Resolving conflicts |
WalkthroughRefactors init command to build a single command string ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
M4dhav
force-pushed
the
fix-181-fix-appwrite-init-site
branch
from
ae8c6df to
30b6bbb
Compare
| }; | ||
|
|
||
| let gitCloneCommands = ""; | ||
| let dirSetupCommands = ""; |
There was a problem hiding this comment.
Changed var name for better differentiation
| if (selected.template === "./") { | ||
| gitCloneCommands = ` | ||
| dirSetupCommands = ` | ||
| cd ${templatesDir} |
There was a problem hiding this comment.
Dynamically reference templatesDir which is already set earlier in the flow
There was a problem hiding this comment.
i dont understand, we are already in the templatesDir here, what is being dynamically referenced?
| let windowsGitCloneCommands = ` | ||
| $tag = (git ls-remote --tags origin "${templateDetails.providerVersion}" | Select-Object -Last 1) -replace '.*refs/tags/', '' | ||
| git fetch --depth=1 origin "refs/tags/$tag" | ||
| git checkout FETCH_HEAD | ||
| `.trim(); | ||
| let unixGitCloneCommands = ` | ||
| git fetch --depth=1 origin refs/tags/$(git ls-remote --tags origin "${templateDetails.providerVersion}" | tail -n 1 | awk -F '/' '{print $3}') | ||
| git checkout FETCH_HEAD | ||
| `.trim(); |
There was a problem hiding this comment.
As Windows cannot use UNIX commands like tail and awk, separate the tag filtering logic
| git checkout FETCH_HEAD | ||
| `.trim(); | ||
|
|
||
| /* Force use CMD as powershell does not support && */ |
There was a problem hiding this comment.
No need to force use CMD, as there are no && operators used in the command
| let usedShell = null; | ||
| if (process.platform === "win32") { | ||
| gitCloneCommands = 'cmd /c "' + gitCloneCommands + '"'; | ||
| dirSetupCommands = dirSetupCommands + "\n" + windowsGitCloneCommands; | ||
| usedShell = "powershell.exe"; | ||
| } | ||
| else { | ||
| dirSetupCommands = dirSetupCommands + "\n" + unixGitCloneCommands; | ||
| } |
There was a problem hiding this comment.
Set shell and final clone commands conditionally
There was a problem hiding this comment.
Left for null on unix as it does not matter what shell is used, but windows MUST use Powershell
| } | ||
|
|
||
| fs.rmSync(path.join(templatesDir, ".git"), { recursive: true }); | ||
| fs.rmSync(path.join(templatesDir, ".git"), { recursive: true, force: true }); |
There was a problem hiding this comment.
Force remove as .git is hidden on Windows
| fs.cpSync(selected.template === "./" | ||
| ? templatesDir | ||
| : path.join(templatesDir, selected.template), | ||
| siteDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
Node 18+ has cpSync so removed the old function which was not executing properly on windows
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@templates/cli/lib/commands/init.ts`:
- Around line 511-520: The command strings assigned to dirSetupCommands include
an unquoted "cd ${templatesDir}" which breaks on paths with spaces; remove the
"cd ${templatesDir}" lines from both branches so dirSetupCommands begins with
the git commands (e.g., "git init", "git remote add origin ${repo}", "git config
...") and rely on the existing execSync call that already supplies cwd:
templatesDir; update the template strings in init.ts where dirSetupCommands is
built to eliminate any leading cd invocation.
- Around line 539-554: The execSync call passes shell: usedShell even when
usedShell is null; change the call in init.ts so the options object only
includes shell when usedShell is a non-null string (or set shell to undefined
instead of null) before calling childProcess.execSync(dirSetupCommands, { stdio:
"pipe", cwd: templatesDir, ... }); specifically update the code that builds the
execSync options for childProcess.execSync to omit the shell property when
usedShell is null to avoid passing null into execSync.
🧹 Nitpick comments (1)
templates/cli/lib/commands/init.ts (1)
513-523: Avoid mutating global Git config for the default branch.
git config --global init.defaultBranch mainruns aftergit init, so it won’t affect the current repo but does permanently change user config. Prefer a repo-scoped setting likegit -c init.defaultBranch=main init(orgit init -b mainif your minimum Git version supports it).♻️ Proposed refactor
- git init - git remote add origin ${repo} - git config --global init.defaultBranch main + git -c init.defaultBranch=main init + git remote add origin ${repo}- git init - git remote add origin ${repo} - git config --global init.defaultBranch main + git -c init.defaultBranch=main init + git remote add origin ${repo}
| dirSetupCommands = ` | ||
| cd ${templatesDir} | ||
| git init | ||
| git remote add origin ${repo} | ||
| git config --global init.defaultBranch main | ||
| git fetch --depth=1 origin refs/tags/$(git ls-remote --tags origin "${templateDetails.providerVersion}" | tail -n 1 | awk -F '/' '{print $3}') | ||
| git checkout FETCH_HEAD | ||
| git config --global init.defaultBranch main | ||
| `.trim(); | ||
| } else { | ||
| gitCloneCommands = ` | ||
| dirSetupCommands = ` | ||
| cd ${templatesDir} | ||
| git init |
There was a problem hiding this comment.
Drop the in‑string cd to avoid path‑with‑spaces failures.
execSync already sets cwd: templatesDir; the unquoted cd ${templatesDir} will fail when the path contains spaces (very common on Windows). Removing it avoids the issue and keeps behavior consistent across shells.
🐛 Proposed fix
- cd ${templatesDir}
git init
git remote add origin ${repo}
git config --global init.defaultBranch main - cd ${templatesDir}
git init
git remote add origin ${repo}
git config --global init.defaultBranch main📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dirSetupCommands = ` | |
| cd ${templatesDir} | |
| git init | |
| git remote add origin ${repo} | |
| git config --global init.defaultBranch main | |
| git fetch --depth=1 origin refs/tags/$(git ls-remote --tags origin "${templateDetails.providerVersion}" | tail -n 1 | awk -F '/' '{print $3}') | |
| git checkout FETCH_HEAD | |
| git config --global init.defaultBranch main | |
| `.trim(); | |
| } else { | |
| gitCloneCommands = ` | |
| dirSetupCommands = ` | |
| cd ${templatesDir} | |
| git init | |
| dirSetupCommands = ` | |
| git init | |
| git remote add origin ${repo} | |
| git config --global init.defaultBranch main | |
| `.trim(); | |
| } else { | |
| dirSetupCommands = ` | |
| git init |
🤖 Prompt for AI Agents
In `@templates/cli/lib/commands/init.ts` around lines 511 - 520, The command
strings assigned to dirSetupCommands include an unquoted "cd ${templatesDir}"
which breaks on paths with spaces; remove the "cd ${templatesDir}" lines from
both branches so dirSetupCommands begins with the git commands (e.g., "git
init", "git remote add origin ${repo}", "git config ...") and rely on the
existing execSync call that already supplies cwd: templatesDir; update the
template strings in init.ts where dirSetupCommands is built to eliminate any
leading cd invocation.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| let usedShell = null; | ||
| if (process.platform === "win32") { | ||
| gitCloneCommands = 'cmd /c "' + gitCloneCommands + '"'; | ||
| dirSetupCommands = dirSetupCommands + "\n" + windowsGitCloneCommands; | ||
| usedShell = "powershell.exe"; | ||
| } | ||
| else { | ||
| dirSetupCommands = dirSetupCommands + "\n" + unixGitCloneCommands; | ||
| } | ||
|
|
||
| /* Execute the child process but do not print any std output */ | ||
| try { | ||
| childProcess.execSync(gitCloneCommands, { | ||
| childProcess.execSync(dirSetupCommands, { | ||
| stdio: "pipe", | ||
| cwd: templatesDir, | ||
| shell: usedShell, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n templates/cli/lib/commands/init.ts | sed -n '530,565p'Repository: appwrite/sdk-generator
Length of output: 1740
🌐 Web query:
Node.js execSync shell option null behavior documentation
💡 Result:
Node’s official docs do not define any special meaning for shell: null—null is simply not a documented/typed value for that option.
- For
child_process.execSync(command[, options]), the docs defineoptions.shellas a<string>, with a default of'/bin/sh'(Unix) orprocess.env.ComSpec(Windows). They do not mentionnullas a way to disable or change behavior. [1] - For
child_process.spawn()/spawnSync(), the docs defineoptions.shellas<boolean> | <string>, defaulting tofalse(no shell). To disable the shell you useshell: false(or omit it); to force a shell you useshell: trueor a shell path string.nullis not documented. [2]
If you’re looking for “no shell” behavior, the documented approaches are:
- use
spawn/spawnSyncwithshell: false(default), or - use
execFile/execFileSyncinstead ofexec/execSync. [2]
Sources:
[1] Node.js child_process.execSync() docs (shell option type + default) (nodejs.org)
[2] Node.js child_process.spawn() / spawnSync() docs (shell option type + default) (nodejs.org)
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://nodejs.org/api/child_process.html?utm_source=openai
Omit shell: usedShell when usedShell is null.
On non‑Windows platforms, usedShell remains null and gets passed to execSync's shell option. Node.js execSync expects shell to be a string (defaulting to '/bin/sh' on Unix or process.env.ComSpec on Windows), not null. Passing an undocumented value risks runtime errors across different Node versions.
🐛 Proposed fix
- let usedShell = null;
+ let usedShell: string | undefined;
if (process.platform === "win32") {
dirSetupCommands = dirSetupCommands + "\n" + windowsGitCloneCommands;
usedShell = "powershell.exe";
}
else {
dirSetupCommands = dirSetupCommands + "\n" + unixGitCloneCommands;
}
/* Execute the child process but do not print any std output */
try {
childProcess.execSync(dirSetupCommands, {
stdio: "pipe",
cwd: templatesDir,
- shell: usedShell,
+ ...(usedShell ? { shell: usedShell } : {}),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let usedShell = null; | |
| if (process.platform === "win32") { | |
| gitCloneCommands = 'cmd /c "' + gitCloneCommands + '"'; | |
| dirSetupCommands = dirSetupCommands + "\n" + windowsGitCloneCommands; | |
| usedShell = "powershell.exe"; | |
| } | |
| else { | |
| dirSetupCommands = dirSetupCommands + "\n" + unixGitCloneCommands; | |
| } | |
| /* Execute the child process but do not print any std output */ | |
| try { | |
| childProcess.execSync(gitCloneCommands, { | |
| childProcess.execSync(dirSetupCommands, { | |
| stdio: "pipe", | |
| cwd: templatesDir, | |
| shell: usedShell, | |
| }); | |
| let usedShell: string | undefined; | |
| if (process.platform === "win32") { | |
| dirSetupCommands = dirSetupCommands + "\n" + windowsGitCloneCommands; | |
| usedShell = "powershell.exe"; | |
| } | |
| else { | |
| dirSetupCommands = dirSetupCommands + "\n" + unixGitCloneCommands; | |
| } | |
| /* Execute the child process but do not print any std output */ | |
| try { | |
| childProcess.execSync(dirSetupCommands, { | |
| stdio: "pipe", | |
| cwd: templatesDir, | |
| ...(usedShell ? { shell: usedShell } : {}), | |
| }); |
🤖 Prompt for AI Agents
In `@templates/cli/lib/commands/init.ts` around lines 539 - 554, The execSync call
passes shell: usedShell even when usedShell is null; change the call in init.ts
so the options object only includes shell when usedShell is a non-null string
(or set shell to undefined instead of null) before calling
childProcess.execSync(dirSetupCommands, { stdio: "pipe", cwd: templatesDir, ...
}); specifically update the code that builds the execSync options for
childProcess.execSync to omit the shell property when usedShell is null to avoid
passing null into execSync.
There was a problem hiding this comment.
execSync's shell param is a String?, and is thus fine with null values passed in.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the TypeScript type definitions for Node.js child_process.execSync
# Look for the shell option type in node_modules/@types/node if available
fd -t f "child_process.d.ts" node_modules 2>/dev/null | head -1 | xargs cat | grep -A 20 "execSync" | head -40
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Alternative: search for ExecSyncOptions or similar type definitions
rg -A 10 "interface.*ExecSyncOptions|interface.*ExecOptions" --type=ts node_modules/@types/node 2>/dev/null | head -60
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check package.json to see what version of `@types/node` is being used
cat package.json | jq '.devDependencies["@types/node"]' 2>/dev/null
Length of output: 44
🌐 Web query:
`@types/node` child_process execSync shell option TypeScript type definition
💡 Result:
In @types/node, child_process.execSync() takes an ExecSyncOptions object where shell is typed as string | undefined (i.e., you can pass a shell path/name, not a boolean). [1]
import { execSync, type ExecSyncOptions } from "node:child_process";
const opts: ExecSyncOptions = { shell: "/bin/bash" };
execSync("echo hi", opts);This matches Node’s docs for execSync, where options.shell is a string: “Shell to execute the command with”. [2] (By contrast, spawn/spawnSync use shell?: boolean | string.) [2][3]
If TypeScript says shell doesn’t exist on the options type, you’re likely on an old @types/node version—upgrade @types/node (and ensure you’re importing from "node:child_process" / using the Node types matching your runtime). [1]
Sources: [1] [2] [3]
@M4dhav, I appreciate your point! Let me clarify what I found:
According to the @types/node type definitions, the shell option for execSync is typed as string | undefined, not string | null | undefined. While Node.js at runtime may be lenient and treat null similarly to undefined, TypeScript will flag passing null as a type error since it's not part of the union type.
The suggested change to use string | undefined and conditionally spread the shell option ensures:
- Type safety - no TypeScript errors
- Explicit intent - clearer that we're omitting the option on Unix rather than passing a null value
- Future compatibility - avoids relying on undocumented runtime behavior
That said, if the code compiles and works for you without TypeScript errors, and you've tested it thoroughly on both platforms, you can keep the current approach. Just be aware that stricter TypeScript configurations might flag this.
Would you prefer to keep the current implementation, or would you like to apply the type-safe version?
🧬 Code Graph Analysis Results
[templates/cli/lib/commands/init.ts: initSite execSync usage]
- Summary description: Shows how execSync is invoked with a potentially null shell option. It defines usedShell as null, conditionally assigns "powershell.exe" on Windows, appends OS-specific git clone commands to dirSetupCommands, and finally calls childProcess.execSync with the shell parameter set to usedShell (which may be null). This documents how a null shell value is tolerated by execSync in this code path.
- Parameters and behavior:
- dirSetupCommands: built as a string of shell commands to initialize and fetch template code.
- usedShell: null by default; set to "powershell.exe" on Windows.
- execSync call:
- path: templatesDir
- options: { stdio: "pipe", cwd: templatesDir, shell: usedShell }
- If an error occurs, specialized error handling is applied based on the error message.
- Location: templates/cli/lib/commands/init.ts, snippet around the OS-detection and execSync invocation for site initialization.
There was a problem hiding this comment.
@M4dhav looks legit too, even tho its optional it can be passed as null it seems
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| git checkout FETCH_HEAD | ||
| `.trim(); | ||
| } | ||
| let windowsGitCloneCommands = ` |
There was a problem hiding this comment.
instead of various variables like this lets have a clean switch statement on process.platform
| dirSetupCommands = ` | ||
| cd ${templatesDir} | ||
| git init | ||
| git remote add origin ${repo} |
There was a problem hiding this comment.
we have an opportunity here to refactor this if else and make this much much cleaner. we can start with a simple string, and use a factory pattern to incrementally add stuff to it, instead of long if else
2 participants
What does this PR do?
This PR Modifies the appwrite init sites command flow to ensure that it works on Windows.
Test Plan
Tested running the command on Windows and Unix machines, and all automated tests are passing as well
Related PRs and Issues
appwrite/sdk-for-cli#181
Have you read the Contributing Guidelines on issues?
yes
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.