Release v0.0 #7
Merged
Release v0.0 #7
Conversation
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
ignore diagramfiles validation
remove create release branch
Added .cursor/rules/cursorrules.mdc and .cursorignore for improved Cursor AI rules and ignore patterns. Removed legacy .cursorrules. Updated README.md with new architecture, build, and validation instructions. Added build.js for domain packaging. Updated and added supporting scripts and configuration files. Various updates to core schemas, tasks, workflows, and mocks for vNext domain structure and testing.
Replaces global vNext CLI commands with local npm scripts for validation and build steps in the GitHub Actions workflow. Removes global installation of vNext CLI and updates commands to use 'npm run validate' and 'npm run build' with appropriate arguments.
F/sprint24
Bump @burgan-tech/vnext-schema dependency from version 0.0.26 to 0.0.27 in package-lock.json for both dependencies and devDependencies.
Update @burgan-tech/vnext-schema to v0.0.27
Introduced a 'tags' array to the get-data-from-workflow.json for better categorization and filtering of the account opening workflow.
Add tags to account opening workflow config
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 23099936 | Triggered | Generic High Entropy Secret | c0b4930 | mockoon/migration-api.json | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Reviewer's GuideIntroduces a configurable vNext domain template with build and setup tooling, schema-based validation, and improved CI integration while aligning the package with the example domain and updating docs/tests accordingly. Sequence diagram for JSON schema validation in validate.jssequenceDiagram
actor Dev
participant NPM as npm_script_validate
participant Validate as validate_js
participant Index as index_js_module
participant Schema as vnext_schema_pkg
participant AJV as Ajv_validator
participant FS as file_system
Dev->>NPM: run npm run validate
NPM->>Validate: node validate_js
Note over Validate: Syntax validation (existing)
Validate->>Index: require index_js_module
Index-->>Validate: getDomainName() and getPathsConfig()
Validate->>Schema: require @burgan_tech_vnext_schema
Schema-->>Validate: getAvailableTypes() and getSchema(type)
Validate->>AJV: new Ajv(allErrors true, strict false, verbose true)
Validate->>AJV: addFormats(ajv)
loop for_each_schema_directory
Validate->>FS: read JSON files under domain
FS-->>Validate: file_contents
Validate->>AJV: validator(json_data)
alt valid
Validate->>Validate: record_passed_file
else invalid
AJV-->>Validate: errors_list
Validate->>Validate: formatErrorMessage(errors, include_line_numbers)
Validate->>Validate: record_failed_file
end
end
Validate->>Validate: print_schema_stats
alt any_failed
Validate->>NPM: exit with non_zero_code
else all_passed
Validate->>NPM: exit 0
end
NPM-->>Dev: show validation summary
Updated class diagram for the vNext template module and configclassDiagram
class VnextTemplateModule {
+getDomainConfig() Object
+getPathsConfig() Object
+getSchemas() Object
+getWorkflows() Object
+getTasks() Object
+getViews() Object
+getFunctions() Object
+getExtensions() Object
+getAvailableTypes() Array
+getDomainName() string
+getComponentPath(componentType string) string
}
class ConfigLoader {
+loadConfig() Object
+getPathsConfig() Object
}
class PathsConfig {
+componentsRoot string
+schemas string
+workflows string
+tasks string
+views string
+functions string
+extensions string
}
class BuildTool {
+parseArgs() Object
+build() void
+getPathsConfig() Object
+findJsonFiles(dirPath string) Array
+getAllFiles(dirPath string) Array
+copyFile(src string, dest string) void
+writeJsonFile(path string, data Object) void
}
class SetupTool {
+isAlreadySetup() bool
+getPathsConfig() Object
+getDomainName() string
+replaceInFile(filePath string, domainName string) bool
+replaceInDirectory(dirPath string, domainName string) void
+renameDomainDirectory(domainName string) bool
+setup() void
}
class SchemaSyncTool {
+syncSchemaVersion() void
}
VnextTemplateModule ..> ConfigLoader : uses
VnextTemplateModule ..> PathsConfig : returns
BuildTool ..> PathsConfig : uses
SetupTool ..> PathsConfig : uses
SchemaSyncTool ..> VnextTemplateModule : reads_config_indirectly
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. WalkthroughThis PR migrates the development and build infrastructure from a mock-server-based setup to a localhost configuration, introduces new orchestration scripts for building and validation, restructures documentation and configuration to be config-driven via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (57)
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 |
![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 and found some issues that need to be addressed.
- In
AccountCreationFailedRule.csx, the second block that checksfailureCategorymistakenly convertserrorCodeinstead offailureCategoryto a string before comparing, which will cause critical category detection to behave incorrectly; this should usefailureCategoryin theConvert.ToString(...)call. getDomainConfiginindex.jsnow returns only the paths configuration instead of the fullvnext.config.json(domain, exports, etc.), which is a behavioral change from the previous API and may break callers expecting the full domain config; consider either restoring the original behavior or adding a separate method for paths while keepinggetDomainConfigas a thin wrapper aroundloadConfig.- In
setup.js, several user-facing messages still include the literal$core(e.g. invalid DOMAIN_NAME error and setup completion text), which will confuse users of custom domain names; these should interpolate the actualdomainNameinstead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AccountCreationFailedRule.csx`, the second block that checks `failureCategory` mistakenly converts `errorCode` instead of `failureCategory` to a string before comparing, which will cause critical category detection to behave incorrectly; this should use `failureCategory` in the `Convert.ToString(...)` call.
- `getDomainConfig` in `index.js` now returns only the paths configuration instead of the full `vnext.config.json` (domain, exports, etc.), which is a behavioral change from the previous API and may break callers expecting the full domain config; consider either restoring the original behavior or adding a separate method for paths while keeping `getDomainConfig` as a thin wrapper around `loadConfig`.
- In `setup.js`, several user-facing messages still include the literal `$core` (e.g. invalid DOMAIN_NAME error and setup completion text), which will confuse users of custom domain names; these should interpolate the actual `domainName` instead.
## Individual Comments
### Comment 1
<location> `core/Workflows/account-opening/src/AccountCreationFailedRule.csx:76-80` </location>
<code_context>
};
-
- failed = failed || failureCodes.Contains(errorCode.ToString());
+ string code = Convert.ToString(errorCode);
+ failed = failed || failureCodes.Contains(code, StringComparer.OrdinalIgnoreCase);
}
</code_context>
<issue_to_address>
**issue (bug_risk):** The failure-category check now uses errorCode instead of failureCategory, which changes the logic and likely breaks the intended condition.
The updated check now compares `criticalCategories` against the error *code* instead of the failure *category*, which alters the rule’s behavior and is likely a bug.
This should still be based on `failureCategory` while using the case-insensitive comparison, for example:
```csharp
string category = Convert.ToString(failureCategory);
failed = failed || criticalCategories.Contains(category, StringComparer.OrdinalIgnoreCase);
```
</issue_to_address>
### Comment 2
<location> `setup.js:103-108` </location>
<code_context>
+ }
+
+ // Check for environment variable (for npm install usage)
+ if (process.env.DOMAIN_NAME) {
+ const domainName = process.env.DOMAIN_NAME.trim();
+ if (validateDomainName(domainName)) {
+ return domainName;
+ } else {
+ console.error(`❌ Invalid domain name in DOMAIN_NAME environment variable: $core`);
+ process.exit(1);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Error message for invalid DOMAIN_NAME is hardcoded to `$core` instead of showing the actual invalid value.
This makes the log misleading and hides the actual value that failed validation. Please interpolate the `domainName` variable instead:
```js
console.error(`❌ Invalid domain name in DOMAIN_NAME environment variable: ${domainName}`);
```
</issue_to_address>
### Comment 3
<location> `setup.js:220` </location>
<code_context>
+ console.log('=======================\n');
+
+ // Get domain name from command line or prompt
+ const domainName = await getDomainName();
+ console.log(`\n📝 Setting up domain: $core\n`);
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Several user-facing messages still hardcode `$core` instead of using the resolved domain name.
These console messages still print `$core` literally even though `domainName` is resolved dynamically. They should interpolate `domainName` instead, for example:
```js
console.log(`\n📝 Setting up domain: ${domainName}\n`);
...
console.log(`\nYour domain "${domainName}" is now configured.`);
```
Suggested implementation:
```javascript
// Get domain name from command line or prompt
```
```javascript
console.log(`\n📝 Setting up domain: ${domainName}\n`);
```
There may be other messages in `setup.js` (for example ones that say something like `Your domain "$core" is now configured.`) that still hardcode `$core`. Those should be updated in the same way to interpolate `domainName`, e.g.:
- `console.log('... $core ...')` → ``console.log(`... ${domainName} ...`)``
- `console.log("Your domain \"$core\" is now configured.")` → ``console.log(`Your domain "${domainName}" is now configured.`)``
Search the file for the literal `$core` and replace those occurrences accordingly.
</issue_to_address>
### Comment 4
<location> `package.json:57-61` </location>
<code_context>
+ "vnext-setup": "setup.js",
+ "vnext-template": "init.js"
+ },
"devDependencies": {
+ "@burgan-tech/vnext-schema": "^0.0.27",
"ajv": "^8.12.0",
"ajv-formats": "^2.1.1"
},
</code_context>
<issue_to_address>
**suggestion:** @burgan-tech/vnext-schema is declared in both dependencies and devDependencies, which is redundant and may cause confusion.
Because this package is used at runtime (e.g., in validate.js, build.js), it only needs to be listed in `dependencies`. Please remove it from `devDependencies` to reflect its intended usage.
```suggestion
"devDependencies": {
"ajv": "^8.12.0",
"ajv-formats": "^2.1.1"
},
```
</issue_to_address>
### Comment 5
<location> `build.js:23-32` </location>
<code_context>
+ return `${colors[color]}${text}${colors.reset}`;
+}
+
+// Load vnext.config.json
+function loadConfig() {
+ try {
+ return JSON.parse(fs.readFileSync('vnext.config.json', 'utf8'));
+ } catch (error) {
+ return null;
+ }
+}
+
+// Get paths configuration with defaults
+function getPathsConfig() {
+ const config = loadConfig();
+ const defaults = {
</code_context>
<issue_to_address>
**suggestion:** Config-loading and paths logic duplicates what index.js already provides; centralizing this would reduce drift and maintenance overhead.
Both `build.js` and `index.js` now define `loadConfig` and `getPathsConfig` with essentially the same behavior, which increases maintenance cost and risk of divergence as config defaults or shape change.
Please import and reuse the existing helpers from `index.js` (or extract them into a shared module) so configuration and path resolution are defined in a single place.
Suggested implementation:
```javascript
dim: '\x1b[2m'
};
const { loadConfig, getPathsConfig } = require('./index');
```
```javascript
function colorize(text, color) {
return `${colors[color]}${text}${colors.reset}`;
}
```
1. Ensure `index.js` actually exports `loadConfig` and `getPathsConfig`, e.g.:
- CommonJS: `module.exports = { loadConfig, getPathsConfig, ... };`
2. If `build.js` no longer uses `fs` directly after this refactor, you can safely remove its `fs` import to keep dependencies minimal.
</issue_to_address>
### Comment 6
<location> `test.js:63` </location>
<code_context>
test('getAvailableTypes returns expected array', () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case that verifies `getAvailableTypes` respects custom paths from `vnext.config.json`
The test currently only covers the default, hard-coded paths. Since `getAvailableTypes` now uses `getPathsConfig` (which can be overridden via `vnext.config.json`), please add a second test that:
1. Creates a temporary `vnext.config.json` with non-default paths (e.g. `{"paths": {"schemas": "DomainSchemas"}}`).
2. Reloads the module (clearing `require.cache` for `./index.js` and the config).
3. Asserts that `getAvailableTypes()` reflects the custom paths rather than the defaults.
This ensures the new config-driven behavior is exercised and guards against regressions in config loading.
</issue_to_address>
### Comment 7
<location> `README.md:110-118` </location>
<code_context>
-**Returns:** `Object` - Key-value pairs of task names and definitions
+This will check:
+- Package.json structure and content
+- Main entry point functionality
+- vnext.config.json validation
</code_context>
<issue_to_address>
**nitpick (typo):** Use the correct casing for the `package.json` filename.
Change `Package.json` to `package.json` to match the actual filename and the casing used elsewhere (e.g. `vnext.config.json`).
```suggestion
This will check:
- package.json structure and content
- Main entry point functionality
- vnext.config.json validation
- Domain directory structure
- JSON file syntax validation
- Schema validation using @burgan-tech/vnext-schema
- Module functionality
- Semantic versioning compliance
```
</issue_to_address>
### Comment 8
<location> `setup.js:38` </location>
<code_context>
const componentsRoot = pathsConfig.componentsRoot;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {componentsRoot} = pathsConfig;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 9
<location> `setup.js:161-169` </location>
<code_context>
if (['.json', '.js', '.md', '.sh', '.txt', '.yml', '.yaml'].includes(ext) ||
entry.name === '.gitignore' ||
entry.name === '.gitattributes') {
if (!processedFiles.has(relativePath)) {
if (replaceInFile(fullPath, domainName)) {
processedFiles.add(relativePath);
}
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if ((['.json', '.js', '.md', '.sh', '.txt', '.yml', '.yaml'].includes(ext) ||
entry.name === '.gitignore' ||
entry.name === '.gitattributes') && !processedFiles.has(relativePath)) {
if (replaceInFile(fullPath, domainName)) {
processedFiles.add(relativePath);
}
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 10
<location> `setup.js:164-168` </location>
<code_context>
if (!processedFiles.has(relativePath)) {
if (replaceInFile(fullPath, domainName)) {
processedFiles.add(relativePath);
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if (!processedFiles.has(relativePath) && replaceInFile(fullPath, domainName)) {
processedFiles.add(relativePath);
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 11
<location> `sync-schema-version.js:11` </location>
<code_context>
const schemaVersion = vnextConfig.schemaVersion;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {schemaVersion} = vnextConfig;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 12
<location> `validate.js:320-359` </location>
<code_context>
if (!inString) {
if (char === '{') {
braceDepth++;
} else if (char === '}') {
braceDepth--;
if (braceDepth < currentDepth) {
// Reset path tracking when exiting a level
if (pathIndex > 0 && braceDepth < pathIndex) {
pathIndex = Math.max(0, pathIndex - 1);
}
}
} else if (char === '[') {
bracketDepth++;
// Check if we're at an array index in the path
if (pathIndex < pathParts.length && /^\d+$/.test(pathParts[pathIndex])) {
const targetIndex = parseInt(pathParts[pathIndex], 10);
if (arrayIndex === targetIndex && bracketDepth === 1) {
pathIndex++;
if (pathIndex === pathParts.length) {
return lineNum + 1;
}
}
}
} else if (char === ']') {
bracketDepth--;
if (bracketDepth === 0) {
arrayIndex++;
}
} else if (char === ':' && currentKey) {
currentKey = '';
} else if (char.match(/[a-zA-Z0-9_]/) && !inString) {
if (i === 0 || line[i-1] === '"' || (i > 0 && line[i-1].match(/[^a-zA-Z0-9_]/))) {
currentKey += char;
}
}
} else {
if (char.match(/[a-zA-Z0-9_]/)) {
currentKey += char;
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge else clause's nested if statement into `else if` ([`merge-else-if`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-else-if))
```suggestion
if (!inString) {
if (char === '{') {
braceDepth++;
} else if (char === '}') {
braceDepth--;
if (braceDepth < currentDepth) {
// Reset path tracking when exiting a level
if (pathIndex > 0 && braceDepth < pathIndex) {
pathIndex = Math.max(0, pathIndex - 1);
}
}
} else if (char === '[') {
bracketDepth++;
// Check if we're at an array index in the path
if (pathIndex < pathParts.length && /^\d+$/.test(pathParts[pathIndex])) {
const targetIndex = parseInt(pathParts[pathIndex], 10);
if (arrayIndex === targetIndex && bracketDepth === 1) {
pathIndex++;
if (pathIndex === pathParts.length) {
return lineNum + 1;
}
}
}
} else if (char === ']') {
bracketDepth--;
if (bracketDepth === 0) {
arrayIndex++;
}
} else if (char === ':' && currentKey) {
currentKey = '';
} else if (char.match(/[a-zA-Z0-9_]/) && !inString) {
if (i === 0 || line[i-1] === '"' || (i > 0 && line[i-1].match(/[^a-zA-Z0-9_]/))) {
currentKey += char;
}
}
}
else if (char.match(/[a-zA-Z0-9_]/)) {
currentKey += char;
}
```
<br/><details><summary>Explanation</summary>Flattening if statements nested within else clauses generates code that is
easier to read and expand upon.
</details>
</issue_to_address>
### Comment 13
<location> `validate.js:325-330` </location>
<code_context>
if (braceDepth < currentDepth) {
// Reset path tracking when exiting a level
if (pathIndex > 0 && braceDepth < pathIndex) {
pathIndex = Math.max(0, pathIndex - 1);
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if (braceDepth < currentDepth && (pathIndex > 0 && braceDepth < pathIndex)) {
pathIndex = Math.max(0, pathIndex - 1);
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 14
<location> `validate.js:350-354` </location>
<code_context>
} else if (char.match(/[a-zA-Z0-9_]/) && !inString) {
if (i === 0 || line[i-1] === '"' || (i > 0 && line[i-1].match(/[^a-zA-Z0-9_]/))) {
currentKey += char;
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
} else if (char.match(/[a-zA-Z0-9_]/) && !inString && (i === 0 || line[i-1] === '"' || (i > 0 && line[i-1].match(/[^a-zA-Z0-9_]/)))) {
currentKey += char;
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 15
<location> `validate.js:381` </location>
<code_context>
let message = err.message;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
let {message} = err;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</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
+76
to
80
| string code = Convert.ToString(errorCode); | ||
| failed = failed || failureCodes.Contains(code, StringComparer.OrdinalIgnoreCase); | ||
| } | ||
|
|
||
| // Check failure category |
There was a problem hiding this comment.
issue (bug_risk): The failure-category check now uses errorCode instead of failureCategory, which changes the logic and likely breaks the intended condition.
The updated check now compares criticalCategories against the error code instead of the failure category, which alters the rule’s behavior and is likely a bug.
This should still be based on failureCategory while using the case-insensitive comparison, for example:
string category = Convert.ToString(failureCategory);
failed = failed || criticalCategories.Contains(category, StringComparer.OrdinalIgnoreCase);
Summary of ChangesHello @yilmaztayfun, 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 significantly refines the vNext workflow development environment by introducing comprehensive documentation, new build and setup tooling, and improved validation mechanisms. It standardizes development practices, updates API mocking configurations, and cleans up project metadata, all aimed at streamlining workflow creation, ensuring consistency, and enhancing the overall developer experience. 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
|
![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 introduces a significant set of changes for the v0.0 release, including new build and setup tooling, configurable component paths, and enhanced validation. The refactoring of the core logic and documentation greatly improves the project's architecture and developer experience. My review has identified one critical bug in a C# workflow rule and a few medium-severity issues in the new documentation and setup script. Overall, these are excellent improvements.
Comment on lines
+91
to
+92
| string code = Convert.ToString(errorCode); | ||
| failed = failed || criticalCategories.Contains(code, StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
There appears to be a copy-paste error in this logic. You are checking the failureCategory, but you are converting and using the errorCode variable from the previous check. This will lead to incorrect logic for evaluating the failure category. You should be converting and using the failureCategory variable.
string categoryCode = Convert.ToString(failureCategory);
failed = failed || criticalCategories.Contains(categoryCode, StringComparer.OrdinalIgnoreCase);
| - Use semantic versioning | ||
|
|
||
| 3. **Create Mockoon mocks** (for external APIs): | ||
| - Add all external API endpoints to `mockoon/banking-api-mocks.json` |
There was a problem hiding this comment.
The documentation refers to an outdated mock file mockoon/banking-api-mocks.json. This file has been replaced by mockoon/migration-api.json. Please update the reference to maintain consistency. Similar outdated references exist on lines 556 and 608.
- Add all external API endpoints to `mockoon/migration-api.json`
| - Determine flow vs subflow vs subprocess usage | ||
|
|
||
| 4. **Create Mockoon mocks** (for external APIs): | ||
| - Add API endpoints to `mockoon/banking-api-mocks.json` |
There was a problem hiding this comment.
The documentation refers to an outdated mock file mockoon/banking-api-mocks.json. This file has been replaced by mockoon/migration-api.json. Please update the reference to maintain consistency.
- Add API endpoints to `mockoon/migration-api.json`
| - **Validation Tool**: Run `npm run validate` after any changes | ||
|
|
||
| ### Testing & Mocking | ||
| - **Mock APIs**: `mockoon/banking-api-mocks.json` - External API mocks (port 3001) |
There was a problem hiding this comment.
The documentation refers to an outdated mock file mockoon/banking-api-mocks.json. This file has been replaced by mockoon/migration-api.json. Please update the reference to maintain consistency.
- **Mock APIs**: `mockoon/migration-api.json` - External API mocks (port 3001)
| if (validateDomainName(domainName)) { | ||
| return domainName; | ||
| } else { | ||
| console.error(`❌ Invalid domain name in DOMAIN_NAME environment variable: $core`); |
There was a problem hiding this comment.
The log message uses a static string $core instead of the actual domain name from the environment variable. This will result in a confusing log message for the user. Please use the process.env.DOMAIN_NAME variable in the template literal.
Suggested change
| console.error(`❌ Invalid domain name in DOMAIN_NAME environment variable: $core`); | |
| console.error(`❌ Invalid domain name in DOMAIN_NAME environment variable: ${process.env.DOMAIN_NAME}`); |
|
|
||
| // Get domain name from command line or prompt | ||
| const domainName = await getDomainName(); | ||
| console.log(`\n📝 Setting up domain: $core\n`); |
There was a problem hiding this comment.
The log message uses a static string $core instead of the domainName variable. This will result in a confusing log message for the user. Please use the actual domain name variable in the template literal.
Suggested change
| console.log(`\n📝 Setting up domain: $core\n`); | |
| console.log(`\n📝 Setting up domain: ${domainName}\n`); |
| renameDomainDirectory(domainName); | ||
|
|
||
| console.log('\n✅ Setup complete!'); | ||
| console.log(`\nYour domain "$core" is now configured.`); |
There was a problem hiding this comment.
The log message uses a static string $core instead of the domainName variable. This will result in a confusing log message for the user. Please use the actual domain name variable in the template literal.
Suggested change
| console.log(`\nYour domain "$core" is now configured.`); | |
| console.log(`\nYour domain "${domainName}" is now configured.`); |
Merged
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 by Sourcery
Add configurable domain/component paths, introduce build and setup tooling, and enhance validation and schema alignment for the vNext template package.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.