Merged
Conversation
* fix: use super class methods * fix: more use of super * fix: more use of super * fix: use ID * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * Update packages/core/src/lib/product/commands/handlers/product.delete.handler.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * Apply suggestion from @cubic-dev-ai[bot] Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * Apply suggestion from @cubic-dev-ai[bot] * Apply suggestion from @cubic-dev-ai[bot] * Apply suggestion from @cubic-dev-ai[bot] * Apply suggestion from @cubic-dev-ai[bot] * fix(core): API build for prod mode * Apply suggestion from @cubic-dev-ai[bot] * Apply suggestion from @cubic-dev-ai[bot] * Apply suggestion from @cubic-dev-ai[bot] --------- Co-authored-by: Rahul R. <rahulrathore576@gmail.com> Co-authored-by: Rahul R. <41804588+rahul-rocket@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
* fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers * fix(build): apply AI suggestion by bot agent reviewers
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Contributor
Greptile SummaryThis PR introduces a major refactoring to improve database operation performance by adding bulk operation methods ( Key Changes:
PR Template Issue: Title Issue: Confidence Score: 5/5
Important Files Changed
Last reviewed commit: b921136 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 96 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/lib/feature/feature-organization.service.ts">
<violation number="1" location="packages/core/src/lib/feature/feature-organization.service.ts:96">
P1: saveMany overwrites each entity’s tenant/tenantId with the current RequestContext tenant. This method builds entries for multiple tenants, so saving with saveMany collapses them all into the current tenant and corrupts tenant associations. Use the raw repository save (or another method that preserves per-entity tenant) here.</violation>
</file>
<file name="packages/core/src/lib/tasks/issue-type/issue-type.service.ts">
<violation number="1" location="packages/core/src/lib/tasks/issue-type/issue-type.service.ts:115">
P1: saveMany overwrites tenant/tenantId with RequestContext.currentTenantId(), so bulkCreateTenantsIssueTypes now ignores the per-tenant values built in issueTypes and creates everything under the current tenant. This breaks multi-tenant seeding.</violation>
</file>
<file name="packages/core/src/lib/tasks/task-status-priority-size.service.ts">
<violation number="1" location="packages/core/src/lib/tasks/task-status-priority-size.service.ts:117">
P2: super.findAll applies tenant scoping and overrides the IsNull() tenant filter in getDefaultEntities, so system defaults (tenantId=null) won’t be returned for authenticated tenants.</violation>
</file>
<file name="packages/core/src/lib/tasks/priorities/priority.service.ts">
<violation number="1" location="packages/core/src/lib/tasks/priorities/priority.service.ts:91">
P1: saveMany overwrites tenant/tenantId with RequestContext.currentTenantId, so bulkCreateTenantsTaskPriorities will persist every priority under the current tenant instead of the per-tenant values built in the loop.</violation>
</file>
<file name="packages/core/src/lib/product-type/product-type.service.ts">
<violation number="1" location="packages/core/src/lib/product-type/product-type.service.ts:62">
P1: The MikroORM fallback path does not enforce `entity.tenantId = tenantId` before saving, unlike the TypeORM path above. This asymmetry can allow an entity with a missing or incorrect `tenantId` to be persisted, breaking tenant isolation.</violation>
</file>
<file name="packages/core/src/lib/product-setting/product-setting.service.ts">
<violation number="1" location="packages/core/src/lib/product-setting/product-setting.service.ts:37">
P2: deleteManySettings always returns the input array instead of the actual delete result, so callers like ProductDeleteHandler will over-count deletions whenever super.deleteMany removes fewer rows than the input size (tenant scoping, missing ids). Return the real DeleteResult/[] or an array that reflects actual deletions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| return await this.typeOrmRepository.save(featureOrganizations); | ||
| return await this.saveMany(featureOrganizations); |
Contributor
There was a problem hiding this comment.
P1: saveMany overwrites each entity’s tenant/tenantId with the current RequestContext tenant. This method builds entries for multiple tenants, so saving with saveMany collapses them all into the current tenant and corrupts tenant associations. Use the raw repository save (or another method that preserves per-entity tenant) here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/lib/feature/feature-organization.service.ts, line 96:
<comment>saveMany overwrites each entity’s tenant/tenantId with the current RequestContext tenant. This method builds entries for multiple tenants, so saving with saveMany collapses them all into the current tenant and corrupts tenant associations. Use the raw repository save (or another method that preserves per-entity tenant) here.</comment>
<file context>
@@ -93,6 +93,6 @@ export class FeatureOrganizationService extends TenantAwareCrudService<FeatureOr
}
- return await this.typeOrmRepository.save(featureOrganizations);
+ return await this.saveMany(featureOrganizations);
}
}
</file context>
|
|
||
| // Save the created or fetched issue types to the repository and return the result. | ||
| return await this.typeOrmRepository.save(issueTypes); | ||
| return await this.saveMany(issueTypes); |
Contributor
There was a problem hiding this comment.
P1: saveMany overwrites tenant/tenantId with RequestContext.currentTenantId(), so bulkCreateTenantsIssueTypes now ignores the per-tenant values built in issueTypes and creates everything under the current tenant. This breaks multi-tenant seeding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/lib/tasks/issue-type/issue-type.service.ts, line 115:
<comment>saveMany overwrites tenant/tenantId with RequestContext.currentTenantId(), so bulkCreateTenantsIssueTypes now ignores the per-tenant values built in issueTypes and creates everything under the current tenant. This breaks multi-tenant seeding.</comment>
<file context>
@@ -112,7 +112,7 @@ export class IssueTypeService extends TaskStatusPrioritySizeService<IssueType> {
// Save the created or fetched issue types to the repository and return the result.
- return await this.typeOrmRepository.save(issueTypes);
+ return await this.saveMany(issueTypes);
} catch (error) {
throw new BadRequestException(
</file context>
| } | ||
| } | ||
| return await this.typeOrmRepository.save(priorities); | ||
| return await this.saveMany(priorities); |
Contributor
There was a problem hiding this comment.
P1: saveMany overwrites tenant/tenantId with RequestContext.currentTenantId, so bulkCreateTenantsTaskPriorities will persist every priority under the current tenant instead of the per-tenant values built in the loop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/lib/tasks/priorities/priority.service.ts, line 91:
<comment>saveMany overwrites tenant/tenantId with RequestContext.currentTenantId, so bulkCreateTenantsTaskPriorities will persist every priority under the current tenant instead of the per-tenant values built in the loop.</comment>
<file context>
@@ -88,7 +88,7 @@ export class TaskPriorityService extends TaskStatusPrioritySizeService<TaskPrior
}
}
- return await this.typeOrmRepository.save(priorities);
+ return await this.saveMany(priorities);
} catch (error) {
throw new BadRequestException(error);
</file context>
Suggested change
| return await this.saveMany(priorities); | |
| return await this.typeOrmRepository.save(priorities); |
| }); | ||
| } | ||
| await super.delete(id); | ||
| return await this.save(entity); |
Contributor
There was a problem hiding this comment.
P1: The MikroORM fallback path does not enforce entity.tenantId = tenantId before saving, unlike the TypeORM path above. This asymmetry can allow an entity with a missing or incorrect tenantId to be persisted, breaking tenant isolation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/lib/product-type/product-type.service.ts, line 62:
<comment>The MikroORM fallback path does not enforce `entity.tenantId = tenantId` before saving, unlike the TypeORM path above. This asymmetry can allow an entity with a missing or incorrect `tenantId` to be persisted, breaking tenant isolation.</comment>
<file context>
@@ -37,8 +40,26 @@ export class ProductTypeService extends TenantAwareCrudService<ProductType> {
+ });
+ }
+ await super.delete(id);
+ return await this.save(entity);
} catch (err) {
throw new BadRequestException(err);
</file context>
Suggested change
| return await this.save(entity); | |
| if (isNotEmpty(tenantId)) { | |
| entity.tenantId = tenantId; | |
| } | |
| return await this.save(entity); |
| // Use base class super.findAll which handles both ORMs every time here because we supply explicit tenant | ||
| // scoping via the where clause (tenantId: RequestContext.currentTenantId() || params.tenantId), so we | ||
| // intentionally bypass any automatic tenant scoping and rely on our explicit filter. | ||
| const result = await super.findAll(options as FindManyOptions<BaseEntity>); |
Contributor
There was a problem hiding this comment.
P2: super.findAll applies tenant scoping and overrides the IsNull() tenant filter in getDefaultEntities, so system defaults (tenantId=null) won’t be returned for authenticated tenants.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/lib/tasks/task-status-priority-size.service.ts, line 117:
<comment>super.findAll applies tenant scoping and overrides the IsNull() tenant filter in getDefaultEntities, so system defaults (tenantId=null) won’t be returned for authenticated tenants.</comment>
<file context>
@@ -112,25 +111,12 @@ export class TaskStatusPrioritySizeService<
+ // Use base class super.findAll which handles both ORMs every time here because we supply explicit tenant
+ // scoping via the where clause (tenantId: RequestContext.currentTenantId() || params.tenantId), so we
+ // intentionally bypass any automatic tenant scoping and rely on our explicit filter.
+ const result = await super.findAll(options as FindManyOptions<BaseEntity>);
+ items = result.items;
+ total = result.total;
</file context>
| if (ids.length > 0) { | ||
| await super.deleteMany(ids); | ||
| } | ||
| return productVariantSettings; |
Contributor
There was a problem hiding this comment.
P2: deleteManySettings always returns the input array instead of the actual delete result, so callers like ProductDeleteHandler will over-count deletions whenever super.deleteMany removes fewer rows than the input size (tenant scoping, missing ids). Return the real DeleteResult/[] or an array that reflects actual deletions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/lib/product-setting/product-setting.service.ts, line 37:
<comment>deleteManySettings always returns the input array instead of the actual delete result, so callers like ProductDeleteHandler will over-count deletions whenever super.deleteMany removes fewer rows than the input size (tenant scoping, missing ids). Return the real DeleteResult/[] or an array that reflects actual deletions.</comment>
<file context>
@@ -14,20 +14,26 @@ export class ProductVariantSettingService extends TenantAwareCrudService<Product
+ if (ids.length > 0) {
+ await super.deleteMany(ids);
+ }
+ return productVariantSettings;
}
}
</file context>
|
View your CI Pipeline Execution ↗ for commit b921136
☁️ Nx Cloud last updated this comment at |
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
2 participants
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.
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by cubic
Standardized CRUD and tenant-aware operations across core services for safer, consistent data access and bulk operations. Also improved Windows CI stability, tightened password rules, and fixed interview feedback behavior.
Refactors
Bug Fixes
Written for commit b921136. Summary will update on new commits.