fix workflow #2
fix workflow #2
Conversation
Reviewer's GuideAdds retry logic to npm installs in the CI workflow, makes the setup script fully non-interactive/CI-safe, and ensures postinstall never fails the install if setup.js exits with an error. Sequence diagram for CI npm install with retry workflowsequenceDiagram
participant GitHubActionsRunner as GitHubActionsRunner
participant InstallStep as InstallDependenciesStep
participant Shell as Shell
participant NPM as npm
GitHubActionsRunner->>InstallStep: start
InstallStep->>Shell: run install_with_retry
loop up to 3 attempts
Shell->>Shell: check for package-lock.json
alt package-lock.json exists
Shell->>NPM: npm ci
else no package-lock.json
Shell->>NPM: npm install
end
alt install succeeds
NPM-->>Shell: success
Shell-->>InstallStep: exit 0
InstallStep-->>GitHubActionsRunner: success
Shell->>Shell: break loop on success
else install fails and attempts remaining
NPM-->>Shell: failure
Shell->>Shell: sleep with exponential backoff
else install fails and no attempts remaining
NPM-->>Shell: failure
Shell-->>InstallStep: exit 1
InstallStep-->>GitHubActionsRunner: failure
end
end
Sequence diagram for postinstall behavior with setup.js in CIsequenceDiagram
participant NPM as npm
participant Postinstall as postinstallScript
participant Setup as setup_js
participant SyncSchema as sync_schema_script
NPM->>Postinstall: run (node setup.js || true) && npm run sync-schema
Postinstall->>Setup: node setup.js
alt setup runs in CI/non-interactive
Setup-->>Postinstall: exit 0 or non-zero (ignored)
else setup runs interactively
Setup-->>Postinstall: exit 0 or non-zero (ignored)
end
Postinstall->>Postinstall: logical OR converts failure to success
Postinstall->>SyncSchema: npm run sync-schema
SyncSchema-->>Postinstall: success or failure (propagated)
Postinstall-->>NPM: overall postinstall status depends only on sync-schema
Flow diagram for non-interactive setup.js behavior in CIflowchart TD
A["start setup"] --> B["isInstalledAsDependency?"]
B -->|yes| C["return (skip setup)"]
B -->|no| D["domainFromConfig = getDomainFromConfig"]
D --> E["isDomainFolderConfigured?"]
E -->|yes| F["isNonInteractive?
(!stdin TTY or CI/GITHUB_ACTIONS/NPM_CONFIG_INTERACTIVE flags)"]
F -->|yes| G["return (skip silently)"]
F -->|no| H["print vNext header and domain already configured messages"]
H --> I["return"]
E -->|no| J["isNonInteractive?"]
J -->|yes| K["does touch file exist?"]
K -->|no| L["return (skip silently)"]
K -->|yes| M["return (skip silently, no prompts)"]
J -->|no| N["print vNext header"]
N --> O["isAlreadySetup?"]
O -->|yes| P["print already set up message and return"]
O -->|no| Q["continue interactive setup prompts"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @middt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the project's setup and post-installation processes to be more robust and compatible with Continuous Integration (CI) environments. It modifies the Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. Note
|
| Cohort / File(s) | Change Summary |
|---|---|
Build Workflow Retry Logic .github/workflows/build-and-publish.yml |
Introduces install_with_retry function that attempts npm install/ci up to 3 times with exponential backoff (5s initial, doubling per retry), using npm ci when package-lock.json exists, otherwise npm install; includes per-attempt progress reporting. |
Setup and Installation Robustness package.json, setup.js |
Modified postinstall script to continue execution via (node setup.js || true) fallback; enhanced setup.js with CI environment detection (CI, GITHUB_ACTIONS flags) to suppress interactive prompts and domain-configured path handling in non-interactive scenarios; adds early guard for missing touch directory and moves setup banner display to interactive mode only. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~12 minutes
- Specific areas requiring attention:
- Bash retry logic in workflow file: verify exponential backoff calculation and retry termination conditions are correct
- CI environment variable detection in setup.js: ensure all relevant CI platform flags are covered
- Fallback behavior in postinstall script: confirm that continuing on setup.js failure doesn't mask legitimate setup errors
Possibly related PRs
- minor fix #1 – Modifies similar setup.js non-interactive and domain-configuration handling logic, including
isNonInteractivechecks andgetDomainNameretrieval paths.
Poem
🐰 Retry with grace, through CI we race,
Exponential backoff at a gentle pace,
Setup runs quiet in pipelines bright,
No prompts to trouble the build's pure flight,
Domain-aware and resilient strong, ✨
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
master
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build-and-publish.yml(1 hunks)package.json(1 hunks)setup.js(2 hunks)
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 @coderabbitai help to get the list of available commands and usage tips.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
setup.js, theisNonInteractive()block that checks forfs.existsSync('touch')always returns regardless of the check, making thetouchfile condition ineffective; either remove the redundant check or adjust the logic so the presence/absence oftouchactually changes behavior. - The change to
postinstall((node setup.js || true) && npm run sync-schema) will mask any real errors fromsetup.jsin local development as well as CI; consider scoping the|| trueto CI-only via an environment check so developers still see failures when setup breaks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `setup.js`, the `isNonInteractive()` block that checks for `fs.existsSync('touch')` always returns regardless of the check, making the `touch` file condition ineffective; either remove the redundant check or adjust the logic so the presence/absence of `touch` actually changes behavior.
- The change to `postinstall` (`(node setup.js || true) && npm run sync-schema`) will mask any real errors from `setup.js` in local development as well as CI; consider scoping the `|| true` to CI-only via an environment check so developers still see failures when setup breaks.
## Individual Comments
### Comment 1
<location> `setup.js:243-252` </location>
<code_context>
const domainFromConfig = getDomainFromConfig();
if (domainFromConfig && isDomainFolderConfigured()) {
+ // In CI/non-interactive mode, skip silently
+ if (isNonInteractive()) {
+ return;
+ }
+ // In interactive mode, show message
+ console.log('🚀 vNext Template Setup');
+ console.log('=======================\n');
console.log(`✅ Domain "${domainFromConfig}" is already configured`);
console.log(` Domain folder "${domainFromConfig}" exists and is set up.`);
console.log(' Skipping setup. If you want to re-run setup, remove the domain directory first.\n');
return;
}
+ // In CI/non-interactive mode, if touch doesn't exist or not configured, skip silently
+ if (isNonInteractive()) {
+ if (!fs.existsSync('touch')) {
+ return;
+ }
+ // If touch exists but we're in CI, skip silently (don't prompt)
+ return;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The `touch` file existence check is currently a no-op due to the unconditional return.
In the `isNonInteractive()` block, this code:
```js
if (!fs.existsSync('touch')) {
return;
}
return;
```
returns in all cases, so the `fs.existsSync('touch')` check and its comment are misleading and effectively dead logic. If `touch` is supposed to change behavior, the second unconditional `return` needs adjusting; if not, this can be simplified to `if (isNonInteractive()) return;` to match the actual behavior.
</issue_to_address>
### Comment 2
<location> `package.json:12` </location>
<code_context>
"sync-schema": "node sync-schema-version.js",
"setup": "node setup.js",
- "postinstall": "node setup.js && npm run sync-schema",
+ "postinstall": "(node setup.js || true) && npm run sync-schema",
"prepublishOnly": "npm run sync-schema && npm run validate"
},
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Swallowing all `setup.js` errors in `postinstall` may hide real failures.
This change makes `postinstall` always succeed even if `setup.js` fails, which can hide real configuration or environment problems on local installs. Consider only relaxing failures in non-interactive contexts (e.g., when `CI`/`GITHUB_ACTIONS` is set) or at least logging a clear warning when `setup.js` exits non‑zero so developers still see and can act on those errors.
```suggestion
"postinstall": "node setup.js || (if [ -n \"$CI\" ] || [ -n \"$GITHUB_ACTIONS\" ]; then echo '[postinstall] Warning: setup.js failed; continuing because CI/GITHUB_ACTIONS is set' >&2; else exit 1; fi) && npm run sync-schema",
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (isNonInteractive()) { | ||
| return; | ||
| } | ||
| // In interactive mode, show message | ||
| console.log('🚀 vNext Template Setup'); | ||
| console.log('=======================\n'); | ||
| console.log(`✅ Domain "${domainFromConfig}" is already configured`); | ||
| console.log(` Domain folder "${domainFromConfig}" exists and is set up.`); | ||
| console.log(' Skipping setup. If you want to re-run setup, remove the domain directory first.\n'); | ||
| return; |
There was a problem hiding this comment.
issue (bug_risk): The touch file existence check is currently a no-op due to the unconditional return.
In the isNonInteractive() block, this code:
if (!fs.existsSync('touch')) {
return;
}
return;returns in all cases, so the fs.existsSync('touch') check and its comment are misleading and effectively dead logic. If touch is supposed to change behavior, the second unconditional return needs adjusting; if not, this can be simplified to if (isNonInteractive()) return; to match the actual behavior.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to improve the CI workflow by making the setup script non-interactive and preventing installation failures. The changes are in the right direction, but I've found a few areas for improvement. The postinstall script change in package.json could mask legitimate errors during interactive setup. In setup.js, the logic for detecting non-interactive environments can be simplified, and the main setup function has some code duplication and convoluted logic that can be refactored for better clarity and maintainability. My detailed comments below provide specific suggestions to address these points.
| "sync-schema": "node sync-schema-version.js", | ||
| "setup": "node setup.js", | ||
| "postinstall": "node setup.js && npm run sync-schema", | ||
| "postinstall": "(node setup.js || true) && npm run sync-schema", |
There was a problem hiding this comment.
The use of || true will suppress any exit code from setup.js, causing the postinstall step to always appear successful even if the setup fails for a legitimate reason (e.g., an invalid domain name provided by a user in an interactive session). This can be misleading as it hides failures. Since setup.js is being modified to exit gracefully in non-interactive environments, this error suppression might not be necessary and could be detrimental in interactive environments. Consider removing || true and ensuring setup.js has the correct exit behavior for all scenarios.
| "postinstall": "(node setup.js || true) && npm run sync-schema", | |
| "postinstall": "node setup.js && npm run sync-schema", |
| return !process.stdin.isTTY || | ||
| process.env.CI === 'true' || | ||
| process.env.GITHUB_ACTIONS === 'true' || | ||
| process.env.NPM_CONFIG_INTERACTIVE === 'false' || | ||
| process.env.CI === '1'; |
There was a problem hiding this comment.
The logic for detecting a non-interactive environment can be simplified. The CI environment variable is set to 'true' in GitHub Actions, so checking for process.env.GITHUB_ACTIONS is redundant. Also, checking for CI being 'true' or '1' can be simplified to just checking for its presence, which is a common and more robust practice. This makes the code cleaner and easier to maintain.
| return !process.stdin.isTTY || | |
| process.env.CI === 'true' || | |
| process.env.GITHUB_ACTIONS === 'true' || | |
| process.env.NPM_CONFIG_INTERACTIVE === 'false' || | |
| process.env.CI === '1'; | |
| return !process.stdin.isTTY || | |
| !!process.env.CI || | |
| process.env.NPM_CONFIG_INTERACTIVE === 'false'; |
| if (domainFromConfig && isDomainFolderConfigured()) { | ||
| // In CI/non-interactive mode, skip silently | ||
| if (isNonInteractive()) { | ||
| return; | ||
| } | ||
| // In interactive mode, show message | ||
| console.log('🚀 vNext Template Setup'); | ||
| console.log('=======================\n'); | ||
| console.log(`✅ Domain "${domainFromConfig}" is already configured`); | ||
| console.log(` Domain folder "${domainFromConfig}" exists and is set up.`); | ||
| console.log(' Skipping setup. If you want to re-run setup, remove the domain directory first.\n'); | ||
| return; | ||
| } | ||
|
|
||
| // In CI/non-interactive mode, if touch doesn't exist or not configured, skip silently | ||
| if (isNonInteractive()) { | ||
| if (!fs.existsSync('touch')) { | ||
| return; | ||
| } | ||
| // If touch exists but we're in CI, skip silently (don't prompt) | ||
| return; | ||
| } | ||
|
|
||
| console.log('🚀 vNext Template Setup'); | ||
| console.log('=======================\n'); |
There was a problem hiding this comment.
This section has some duplicated code and complex logic. The header is printed in two separate places, and the check for non-interactive mode is split and contains a redundant conditional. This can be refactored to be more readable and maintainable by handling the non-interactive case upfront, and then printing the header once for all interactive scenarios.
if (isNonInteractive()) {
// In non-interactive mode (e.g. CI), we should not perform any setup
// or display any output. The script should exit gracefully.
return;
}
// Interactive setup starts here
console.log('🚀 vNext Template Setup');
console.log('=======================\n');
if (domainFromConfig && isDomainFolderConfigured()) {
console.log(`✅ Domain "${domainFromConfig}" is already configured`);
console.log(` Domain folder "${domainFromConfig}" exists and is set up.`);
console.log(' Skipping setup. If you want to re-run setup, remove the domain directory first.\n');
return;
}
Summary by Sourcery
Improve installation robustness in CI and adjust non-interactive setup behavior.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.