Feat/sales channel retrieve#2
Conversation
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaAPI: Added Service Layer: Implemented Repository: Extended Types: Added Files Changed
Review Focus Areas
ArchitectureDesign Decisions: Used Risks:
Merge StatusNOT MERGEABLE — PR Score 59/100, below threshold (50)
|
Workflow DiagramsAutomatically generated sequence diagrams showing the workflows in this PR 1. Sales Channel Retrieval API FlowMedium complexity • Components: SalesChannelService, SalesChannelRepository, getRequestedSalesChannel middleware sequenceDiagram
title Sales Channel Retrieve API Endpoint
participant Client as Client
participant API as Admin API Route
participant Middleware as getRequestedSalesChannel Middleware
participant Service as SalesChannelService
participant Repo as SalesChannelRepository
participant DB as Database
Client->>API: GET /admin/sales-channels/:id
API->>Middleware: Route with middleware
Middleware->>Service: retrieve(salesChannelId, config)
Service->>Service: atomicPhase_ (transaction wrapper)
Service->>Repo: getCustomRepository()
Service->>Repo: findOneWithRelations(relations, query)
Repo->>DB: SELECT with WHERE id = salesChannelId
DB-->>Repo: SalesChannel entity
Repo-->>Service: SalesChannel or undefined
alt SalesChannel not found
Service-->>Middleware: throw MedusaError NOT_FOUND
Middleware-->>API: Error propagated
API-->>Client: 404 Not Found
else SalesChannel found
Service-->>Middleware: Return SalesChannel
Middleware->>Middleware: Attach to req.sales_channel
Middleware-->>API: next()
API-->>Client: 200 OK with {sales_channel: {...}}
end
Note over Client,DB: Integration test flow uses sales-channels-seeder.js to seed test data before each test
Note: Diagrams show detected patterns only. Complex workflows may require manual review. |
![codity-chait[bot]](https://avatars.githubusercontent.com/u/228176587?s=60&v=4){kind=small}
| import { SalesChannel } from "@medusajs/medusa/dist/models/sales-channel" | ||
|
|
||
| module.exports = async (connection, data = {}) => { |
There was a problem hiding this comment.
The seeder mixes ES module import syntax with CommonJS module.exports, causing a runtime SyntaxError when the test infrastructure uses require() to load it. All other seeders in the codebase use pure CommonJS with const X = require(...), and the integration test environment has no Babel transform configured. This breaks the GET /admin/sales-channels/:id test suite.
Suggested fix
const { SalesChannel } = require("@medusajs/medusa/dist/models/sales-channel")
module.exports = async (connection, data = {}) => {Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: integration-tests/api/helpers/sales-channels-seeder.js
Lines: 1-3
Issue Type: functional-critical
Severity: critical
Issue Description:
The seeder mixes ES module `import` syntax with CommonJS `module.exports`, causing a runtime `SyntaxError` when the test infrastructure uses `require()` to load it. All other seeders in the codebase use pure CommonJS with `const X = require(...)`, and the integration test environment has no Babel transform configured. This breaks the GET /admin/sales-channels/:id test suite.
Current Code:
import { SalesChannel } from "@medusajs/medusa/dist/models/sales-channel"
module.exports = async (connection, data = {}) => {
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| import EventBusService from "./event-bus" | ||
| import { buildQuery } from "../utils" | ||
| import { FindWithoutRelationsOptions } from "../repositories/product" | ||
| import { MedusaError } from "medusa-core-utils/dist" |
There was a problem hiding this comment.
The import statement on line 13 imports MedusaError from the internal dist directory rather than the package's public entry point. This bypasses the package's intended API contract and creates a fragile dependency that will break silently if the maintainers reorganize the internal file structure. The correct approach is to import from the public entry point medusa-core-utils.
| import { MedusaError } from "medusa-core-utils/dist" | |
| import { MedusaError } from "medusa-core-utils" |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/medusa/src/services/sales-channel.ts
Lines: 13-13
Issue Type: robustness-high
Severity: high
Issue Description:
The import statement on line 13 imports `MedusaError` from the internal `dist` directory rather than the package's public entry point. This bypasses the package's intended API contract and creates a fragile dependency that will break silently if the maintainers reorganize the internal file structure. The correct approach is to import from the public entry point `medusa-core-utils`.
Current Code:
import { MedusaError } from "medusa-core-utils/dist"
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| eventBusService: eventBusServiceMock, | ||
| batchJobRepository: batchJobRepositoryMock | ||
| }) | ||
| } as any) |
There was a problem hiding this comment.
The as any cast on line 25 violates the project's AGENTS.md prohibition against type-unsafe casts. This mask a type mismatch between the mock object and the actual BatchJobService constructor parameter type, making it impossible to catch real type errors during development. Removing the cast requires properly typing the mock object to match the expected service dependencies interface, ensuring type safety and maintainability.
| } as any) | |
| } as BatchJobService['constructor']['prototype']['constructor']) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/medusa/src/services/__tests__/batch-job.ts
Lines: 25-25
Issue Type: maintainability-low
Severity: low
Issue Description:
The `as any` cast on line 25 violates the project's AGENTS.md prohibition against type-unsafe casts. This mask a type mismatch between the mock object and the actual `BatchJobService` constructor parameter type, making it impossible to catch real type errors during development. Removing the cast requires properly typing the mock object to match the expected service dependencies interface, ensuring type safety and maintainability.
Current Code:
} as any)
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Nitpicks (Low Priority)Found 3 low-priority suggestions for code improvement Click to expand nitpicks
|
Security Scan Summary
No critical security issues detected Scan completed in 36.4sSecurity scan powered by Codity.ai |
License Compliance Scan
Strong copyleft licenses detected - review before merging Weak copyleft licenses found - verify compatibility Some packages have unknown licenses - manual review required High Risk Licenses - 4 packages(BSD-3-Clause OR GPL-2.0) (1 packages):
AGPL-3.0-or-later (2 packages):
GPL-2.0-only (1 packages):
Medium Risk Licenses - 4 packages(MPL-2.0 OR Apache-2.0) (2 packages):
MPL-2.0 (2 packages):
Unknown Licenses - 1734 packages
...and 1714 more Powered by Codity.ai · Docs |
Code Quality Report — test-org-codity/medusa · PR #2Scanned: 2026-06-06 17:24 UTC | Score: 62/100 | Provider: github Executive Summary
Top Findings[CQ-LLM-002]
|
| File | Critical | High | Medium | Low | Total |
|---|---|---|---|---|---|
integration-tests/api/__tests__/admin/sales-channels.js |
0 | 1 | 1 | 1 | 3 |
packages/medusa/src/api/middlewares/sales-channel/get-requested-sales-channel.ts |
0 | 0 | 1 | 0 | 1 |
packages/medusa/src/api/routes/admin/sales-channels/get-sales-channels.ts |
0 | 0 | 1 | 1 | 2 |
packages/medusa/src/api/routes/admin/sales-channels/index.ts |
0 | 0 | 0 | 1 | 1 |
Recommendations
- Resolve High severity issues, especially error handling gaps and performance bottlenecks.
- Run automated tests after applying fixes to verify no regressions.
|
@codity review |
PR SummaryWhat Changed
Key Changes by AreaAPI Routes: New endpoint with nested router pattern and Service Layer: Repository: Added relation-aware query methods to Types: Added Testing: Unit tests for service and route handler, plus integration tests with snapshot validation. Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The nested router pattern with middleware pre-fetching keeps handlers thin and reusable. The Risks: The typo in the error message is unintentional and should be fixed before merge. It affects client-visible error responses. Merge StatusNOT MERGEABLE — PR Score 50/100, below threshold (50)
|
Workflow DiagramsAutomatically generated sequence diagrams showing the workflows in this PR 1. Sales Channel Retrieval API FlowMedium complexity • Components: SalesChannelService, SalesChannelRepository, getRequestedSalesChannel middleware sequenceDiagram
title Sales Channel Retrieve API Workflow
participant Client as HTTP Client
participant API as Admin API Route
participant Middleware as getRequestedSalesChannel Middleware
participant Service as SalesChannelService
participant Repo as SalesChannelRepository
participant DB as Database
Client->>API: GET /admin/sales-channels/:id
API->>Middleware: Invoke middleware with id param
Middleware->>Service: retrieve(salesChannelId, config)
note over Middleware,Service: Extracts ID from request params
Service->>Service: atomicPhase_ (transaction wrapper)
Service->>Repo: getCustomRepository(SalesChannelRepository)
Service->>Repo: findOneWithRelations(relations, query)
note over Service,Repo: Builds query with buildQuery utility
Repo->>DB: SELECT with optional relations
DB-->>Repo: SalesChannel entity or null
Repo-->>Service: saleChannel or undefined
alt Sales channel not found
Service->>Service: throw MedusaError NOT_FOUND
Service-->>Middleware: Error propagated
Middleware-->>API: Error propagated
API-->>Client: 404 Not Found
else Sales channel found
Service-->>Middleware: Return SalesChannel entity
Middleware->>Middleware: Attach to req.salesChannel
Middleware-->>API: next()
API->>API: Format response
API-->>Client: 200 OK with sales_channel data
end
note over Client,DB: Integration Test Flow
participant Test as Integration Test
participant Seeder as salesChannelsSeeder
Test->>Seeder: Seed test data
Seeder->>DB: INSERT sales_channel_1
Test->>API: GET /admin/sales-channels/sales_channel_1
API-->>Test: 200 with matching snapshot
Note: Diagrams show detected patterns only. Complex workflows may require manual review. |
| import { SalesChannel } from "@medusajs/medusa/dist/models/sales-channel" | ||
|
|
||
| module.exports = async (connection, data = {}) => { |
There was a problem hiding this comment.
The file uses ES module import syntax on line 1 but exports using CommonJS module.exports on line 3. Since the file is .js without "type": "module" in package.json, Node.js treats it as CommonJS, causing a syntax error when the file is parsed. This breaks the integration test suite when salesChannelsSeeder is required.
Suggested fix
const { SalesChannel } = require("@medusajs/medusa/dist/models/sales-channel")
module.exports = async (connection, data = {}) => {Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: integration-tests/api/helpers/sales-channels-seeder.js
Lines: 1-3
Issue Type: functional-critical
Severity: critical
Issue Description:
The file uses ES module `import` syntax on line 1 but exports using CommonJS `module.exports` on line 3. Since the file is `.js` without `"type": "module"` in `package.json`, Node.js treats it as CommonJS, causing a syntax error when the file is parsed. This breaks the integration test suite when `salesChannelsSeeder` is required.
Current Code:
import { SalesChannel } from "@medusajs/medusa/dist/models/sales-channel"
module.exports = async (connection, data = {}) => {
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| import EventBusService from "./event-bus" | ||
| import { buildQuery } from "../utils" | ||
| import { FindWithoutRelationsOptions } from "../repositories/product" | ||
| import { MedusaError } from "medusa-core-utils/dist" |
There was a problem hiding this comment.
MedusaError is imported from the internal dist subdirectory instead of the public package entry point. This breaks the established pattern used across 50+ other files in the codebase and violates the package's public API contract, risking silent failures if the package reorganizes its compiled output.
| import { MedusaError } from "medusa-core-utils/dist" | |
| import { MedusaError } from "medusa-core-utils" |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: packages/medusa/src/services/sales-channel.ts
Lines: 13-13
Issue Type: maintainability-high
Severity: high
Issue Description:
MedusaError is imported from the internal dist subdirectory instead of the public package entry point. This breaks the established pattern used across 50+ other files in the codebase and violates the package's public API contract, risking silent failures if the package reorganizes its compiled output.
Current Code:
import { MedusaError } from "medusa-core-utils/dist"
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| beforeAll(async () => { | ||
| const cwd = path.resolve(path.join(__dirname, "..", "..")) | ||
| const [process, connection] = await startServerWithEnvironment({ | ||
| process.env.MEDUSA_FF_SALES_CHANNELS = true |
There was a problem hiding this comment.
Line 26 assigns a boolean true to process.env.MEDUSA_FF_SALES_CHANNELS, but environment variables are always strings in Node.js. This causes the value to coerce to the string "true", which will fail strict equality checks (=== true) elsewhere in the code. Additionally, this direct mutation of process.env is redundant since the env object passed to startServerWithEnvironment on line 29 already handles the subprocess environment configuration.
| process.env.MEDUSA_FF_SALES_CHANNELS = true | |
| // Remove line 26 entirely; the env object on line 29 already handles the subprocess |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: integration-tests/api/__tests__/admin/sales-channels.js
Lines: 26-26
Issue Type: functional-medium
Severity: medium
Issue Description:
Line 26 assigns a boolean `true` to `process.env.MEDUSA_FF_SALES_CHANNELS`, but environment variables are always strings in Node.js. This causes the value to coerce to the string `"true"`, which will fail strict equality checks (`=== true`) elsewhere in the code. Additionally, this direct mutation of `process.env` is redundant since the `env` object passed to `startServerWithEnvironment` on line 29 already handles the subprocess environment configuration.
Current Code:
process.env.MEDUSA_FF_SALES_CHANNELS = true
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| beforeEach(async() => { | ||
| try { | ||
| await adminSeeder(dbConnection) | ||
| await salesChannelsSeeder(dbConnection) | ||
| } catch (e) { | ||
| console.error(e) | ||
| } |
There was a problem hiding this comment.
The beforeEach block catches and silently logs seeder errors with console.error, allowing tests to continue without required fixture data. When seeding fails (e.g., due to import/export issues), subsequent tests fail with cryptic errors unrelated to the actual setup failure, making CI triage difficult.
Suggested fix
beforeEach(async() => {
await adminSeeder(dbConnection)
await salesChannelsSeeder(dbConnection)
})Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert bash developer with deep knowledge of security, performance, and best practices.
### Context
File: integration-tests/api/__tests__/admin/sales-channels.js
Lines: 51-57
Issue Type: robustness-low
Severity: low
Issue Description:
The `beforeEach` block catches and silently logs seeder errors with `console.error`, allowing tests to continue without required fixture data. When seeding fails (e.g., due to import/export issues), subsequent tests fail with cryptic errors unrelated to the actual setup failure, making CI triage difficult.
Current Code:
beforeEach(async() => {
try {
await adminSeeder(dbConnection)
await salesChannelsSeeder(dbConnection)
} catch (e) {
console.error(e)
}
})
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow bash best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Nitpicks (Low Priority)Found 1 low-priority suggestions for code improvement Click to expand nitpicks
|
Security Scan Summary
No critical security issues detected Scan completed in 21.2sSecurity scan powered by Codity.ai |
License Compliance Scan
Strong copyleft licenses detected - review before merging Weak copyleft licenses found - verify compatibility Some packages have unknown licenses - manual review required High Risk Licenses - 4 packages(BSD-3-Clause OR GPL-2.0) (1 packages):
AGPL-3.0-or-later (2 packages):
GPL-2.0-only (1 packages):
Medium Risk Licenses - 4 packages(MPL-2.0 OR Apache-2.0) (2 packages):
MPL-2.0 (2 packages):
Unknown Licenses - 1734 packages
...and 1714 more Powered by Codity.ai · Docs |
Code Quality Report — test-org-codity/medusa · PR #2Scanned: 2026-06-06 18:06 UTC | Score: 62/100 | Provider: github Executive Summary
Top Findings[CQ-LLM-002]
|
| File | Critical | High | Medium | Low | Total |
|---|---|---|---|---|---|
integration-tests/api/__tests__/admin/sales-channels.js |
0 | 1 | 1 | 1 | 3 |
packages/medusa/src/api/middlewares/sales-channel/get-requested-sales-channel.ts |
0 | 0 | 1 | 0 | 1 |
packages/medusa/src/api/routes/admin/sales-channels/get-sales-channels.ts |
0 | 0 | 1 | 1 | 2 |
packages/medusa/src/api/routes/admin/sales-channels/index.ts |
0 | 0 | 0 | 1 | 1 |
Recommendations
- Resolve High severity issues, especially error handling gaps and performance bottlenecks.
- Run automated tests after applying fixes to verify no regressions.
4 participants
Summary
What — What changes are introduced in this PR?
Please provide answer here
Why — Why are these changes relevant or necessary?
Please provide answer here
How — How have these changes been implemented?
Please provide answer here
Testing — How have these changes been tested, or how can the reviewer test the feature?
Please provide answer here
Examples
Provide examples or code snippets that demonstrate how this feature works, or how it can be used in practice.
This helps with documentation and ensures maintainers can quickly understand and verify the change.
// Example usageChecklist
Please ensure the following before requesting a review:
yarn changesetand follow the promptsAdditional Context
Add any additional context, related issues, or references that might help the reviewer understand this PR.