-
Notifications
You must be signed in to change notification settings - Fork 8
Feature/cli optimise #598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/cli optimise #598
Conversation
4a0510d to
7cd75ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 847 out of 852 changed files in this pull request and generated no comments.
Files not reviewed (5)
- .vscode/extensions.json: Language not supported
- api/.gitignore: Language not supported
- api/package.json: Language not supported
- api/production.env: Language not supported
- api/tsconfig.json: Language not supported
Comments suppressed due to low confidence (1)
api/src/server.ts:127
- Consider renaming the parameter 'path' in the file watcher callback to avoid shadowing the imported 'path' module, for clarity.
watcher.on('change', async (path) => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 847 out of 852 changed files in this pull request and generated 1 comment.
Files not reviewed (5)
- .vscode/extensions.json: Language not supported
- api/.gitignore: Language not supported
- api/package.json: Language not supported
- api/production.env: Language not supported
- api/tsconfig.json: Language not supported
Comments suppressed due to low confidence (1)
api/src/utils/custom-logger.utils.ts:10
- Using startsWith for path validation could be error prone; replacing it with a check using path.relative would ensure that the resolved path truly resides within the basePath.
if (!resolvedPath.startsWith(basePath)) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 847 out of 852 changed files in this pull request and generated no comments.
Files not reviewed (5)
- .vscode/extensions.json: Language not supported
- api/.gitignore: Language not supported
- api/package.json: Language not supported
- api/production.env: Language not supported
- api/tsconfig.json: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 848 out of 853 changed files in this pull request and generated no comments.
Files not reviewed (5)
- .vscode/extensions.json: Language not supported
- api/.gitignore: Language not supported
- api/package.json: Language not supported
- api/production.env: Language not supported
- api/tsconfig.json: Language not supported
Comments suppressed due to low confidence (2)
api/src/validators/stack.validator.ts:14
- [nitpick] Consider adding a brief comment explaining the use of a global regex in error message replacement, to clarify its necessity compared to a single replacement.
errorMessage: VALIDATION_ERRORS.STRING_REQUIRED.replace(/\$/g, "Name"),
api/src/utils/custom-logger.utils.ts:25
- [nitpick] Review the directory containment check logic in safeJoin to ensure that all edge cases for preventing directory traversal are correctly handled. Adding inline comments to explain the rationale could improve maintainability.
if (relativePath === '' || relativePath === '.' || (!relativePath.startsWith('..') && !path.isAbsolute(relativePath))) {
…r into feature/cli-optimise
…r into feature/cli-optimise
…ion-v2-node-server into feature/cli-optimise
| setExistingField((prevExisting: ExistingFieldType) => { | ||
| const updatedExisting = { ...prevExisting }; | ||
| const validLabels = cmsLocaleOptions?.map(locale => locale.label) || []; | ||
| Object.entries(updatedExisting).forEach(([index, entry]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const validLabels = cmsLocaleOptions?.map(locale => locale?.label) || [];
Object.entries(updatedExisting)?.forEach(([index, entry]) => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| Object.entries(updatedExisting).forEach(([index, entry]) => { | ||
| const [labelPart] = entry.label.split('-'); | ||
| if (!validLabels.includes(labelPart)) { | ||
| delete updatedExisting[index]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const [labelPart] = entry?.label?.split('-');
if (!validLabels?.includes(labelPart)) {
delete updatedExisting?.[index];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entry?.label?.split('-'); adding validation to this gives error, added validation to other code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| Object.keys(updated).forEach((key) => { | ||
| const [labelPart] = key.split('-'); | ||
| if (!validLabels.includes(labelPart)) { | ||
| delete updated[key]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.keys(updated)?.forEach((key) => {
const [labelPart] = key?.split('-');
if (!validLabels?.includes(labelPart)) {
delete updated?.[key];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| const isPresent = prevList.some( | ||
| (item: { label: string; value: string }) => item?.value === 'master_locale' | ||
| const isPresent = prevList.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prevList?.filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
|
||
| if (!isPresent) { | ||
|
|
||
| if(isPresent[0]?.label !== newLabel){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(isPresent?.[0]?.label !== newLabel){
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
No description provided.