add test file

[codevon]

No description provided.

[github-actions]

AI Code Review Inline Comments

[github-actions]

⚠️ SUGGESTION

Score: 7/10
Issue: Inconsistent error handling strategy: Some methods return {success: false, message: ...} objects while others throw errors. This inconsistent pattern makes it difficult for clients to handle errors uniformly.

Suggestion:

Choose one error handling strategy and apply it consistently. Option A - Exception-based (Recommended for NestJS): @Get('ventrata/resellers/search')
async searchVentrataResellers(@Query('iataCode') iataCode: string) {
  if (!iataCode) {
    throw new BadRequestException('iataCode query parameter is required');
  }
  // ... rest of logic
} Option B - Result object-based: All methods should return {success, data, message} and never throw.

Reasoning: Violates Step 2: Architectural & Design Review - Error Handling Strategy. Project conventions Section 11 state that error handling should follow an established project pattern. Global conventions Section 7.1 require Don't disrupt main logic with error handling: Isolate with guards.

[github-actions]

⚠️ SUGGESTION

Score: 7/10
Issue: Redundant array access check: Redundant array access makes logic confusing. If datasetItems.length === 0, datasetItems[0] is undefined, but we check it anyway.

Suggestion:

Handle each case distinctly with early returns:
const datasetItems = await this.apifyClientService.getDatasetItems(...);

if (datasetItems.length === 0) {
  this.logger.warn(`No dataset items found for succeeded run ${runId}, treating as failed`);
  return { success: false, data: [], message: '...' };
}

const firstItem = datasetItems[0];
if (firstItem.error && !hasAcceptableError(firstItem.error)) {
  this.logger.warn(`Dataset contains unexpected error: ${firstItem.error}`);
  return { success: false, data: datasetItems, message: '...' };
}

return { success: true, data: datasetItems, message: '...' };

Reasoning: Violates conventions Section 3.1: Linear code - read top to bottom. Reduces cognitive load (Section 1: Readability trumps all). Early returns vs. flag-based logic.

[github-actions]

⚠️ SUGGESTION

Score: 7/10
Issue: Extract complex Apify status logic into function: Method is too long with deeply nested logic (3 levels). Per conventions Section 3.1, >10 lines of nested logic should be extracted.

Suggestion:

Extract to separate methods:
async getApifyRunStatus(@Query('runId') runId: string) {
  if (!runId) return { success: false, message: 'runId is required' };

  try {
    this.logger.log(`Getting Apify run status for runId: ${runId}`);
    const runResult = await this.apifyClientService.getRunStatus(runId);

    return await this.evaluateApifyRunResult(runResult, runId);
  } catch (error) {
    return this.handleError(error, `Apify run status check for ${runId}`);
  }
}

private async evaluateApifyRunResult(runResult: ApifyRunResult, runId: string): Promise<ControllerResponse<ApifyPromptPilotDatasetItem[]>> {
  // Extract validation logic here
}

Reasoning: Per conventions Section 3: Functions should tell a story. Each extracted function has single responsibility. Makes testing individual logic paths possible. Improves readability dramatically.

[github-actions]

⚠️ SUGGESTION

Score: 7/10
Issue: Insufficient input validation: Query parameters not validated for format/length. IATA codes should be exactly 2 characters. RunIds and RegistrationIds should be validated format.

Suggestion:

Define validators and apply guards:
const isValidIataCode = (code: string): boolean => {
  return /^[A-Z]{2}$/.test(code); // 2 uppercase letters
};

const isValidRunId = (id: string): boolean => {
  return /^[a-zA-Z0-9_-]{10,}$/.test(id); // Minimum length, valid characters
};

// Apply guards
async searchVentrataResellers(@Query('iataCode') iataCode: string) {
  if (!iataCode || !isValidIataCode(iataCode)) {
    return {
      success: false,
      message: 'Invalid IATA code. Must be 2 uppercase letters (e.g., "US")',
    };
  }
  // ...
}

Reasoning: Step 4: Security - Injection prevention. Reduces downstream errors in third-party services. Makes API contract clearer to clients.

[github-actions]

⚠️ SUGGESTION

Score: 6/10
Issue: Generic variable name: Variable named result is too generic and doesn't convey its meaning. After reading the entire method, it's clear this represents whether the Apify job completed successfully.

Suggestion:

let isApifyJobSuccessful = true;
if (datasetItems.length === 0) {
  isApifyJobSuccessful = false;
  // ...
}

return {
  success: isApifyJobSuccessful,
  // ...
};

Reasoning: Violates Step 3: Implementation Review - Readability. Global conventions Section 2.1 explicitly forbids generic names like temp, data, obj - and result falls into this category. Section 2.2 states Names Are Documentation.

[github-actions]

⚠️ SUGGESTION

Score: 6/10
Issue: Magic string in business logic: The string 'panelAgencyResult2Columns' is a magic constant embedded in business logic. It's unclear what this represents or why this specific error should be treated differently.

Suggestion:

// At class or module level
private readonly ACCEPTABLE_APIFY_ERROR_MARKERS = ['panelAgencyResult2Columns'];

// In method
private isAcceptableApifyError(error: string | undefined): boolean {
  if (!error) return false;
  return this.ACCEPTABLE_APIFY_ERROR_MARKERS.some(marker => error.includes(marker));
}

// Usage
const firstItemError = datasetItems[0]?.error;
if (firstItemError && !this.isAcceptableApifyError(firstItemError)) {
  isApifyJobSuccessful = false;
  this.logger.warn(`Dataset item contains unacceptable error for jobId ${runId}, treating as failed`);
}

Reasoning: Violates Step 3: Implementation Review - Readability. Global conventions Section 2.1 require Summarize Complex Expressions: Name intermediate results and Section 3.1 states functions should tell a story. Magic strings should be extracted to named constants with explanatory comments.

[github-actions]

⚠️ SUGGESTION

Score: 6/10
Issue: Missing return type annotations: Some async methods lack explicit return type annotations, making it harder to understand the contract and catch type errors.

Suggestion:

interface VentrataSearchResponse {
  success: boolean;
  data?: any[];
  exists?: boolean;
  message: string;
}

@Get('ventrata/resellers/search')
async searchVentrataResellers(@Query('iataCode') iataCode: string): Promise<VentrataSearchResponse> {
  // Method body
}

Reasoning: Violates Step 3: Implementation Review - Best Practices. TypeScript best practices recommend explicit return types for public API methods. The project conventions Section 7.3 show examples of explicit return types (e.g., Promise), making this inconsistency a style violation.

[github-actions]

⚠️ SUGGESTION

Score: 6/10
Issue: Verbose logging exposes sensitive data: Logs include user inputs and PII directly, creating potential privacy exposure.

Suggestion:

this.logger.log('Searching Ventrata resellers');
this.logger.error('Search failed', { iataCode: iataCode.substring(0, 1) + '*' }); // Partial mask
this.logger.log('Registering agent and agency');

Reasoning: Step 4: Security - Sensitive data exposure. Even in dev endpoints, logs might be aggregated/monitored. Per conventions Section 7: Error handling should sanitize messages.

[github-actions]

⚠️ SUGGESTION

Score: 6/10
Issue: Error messages leak implementation details: Error messages directly expose stack traces and implementation details to clients.

Suggestion:

catch (error) {
  this.logger.error(`Search failed`, error); // Full error to logs
  return {
    success: false,
    message: 'Search operation failed. Please try again later.',
    code: 'SEARCH_FAILED'
  };
}

Reasoning: Step 4: Security - Sensitive data exposure. Per conventions Section 7.1: Error handling should diagnose but not expose internals. Client needs error code to handle, not stack trace.

[github-actions]

⚠️ SUGGESTION

Score: 6/10
Issue: Inconsistent response data types: Different methods return different data types for data field (null vs. array vs. objects).

Suggestion:

Standardize all to return array or object, never null:
interface ControllerResponse<T = any> {
  success: boolean;
  data: T;
  message: string;
}

// Usage: null becomes empty array or default object
return { success: false, data: [], message: '...' }; // Empty array for lists

Reasoning: API contract inconsistency confuses clients. Violates conventions Section 8.2: Encapsulate data + behavior; API should be predictable.

[github-actions]

AI Code Review Inline Comments

[github-actions]

⚠️ SUGGESTION

Score: 8/10
Issue: Business logic in controller: Complex Apify-specific business logic (checking dataset items, validating error strings) belongs in the service layer, not the controller.

Suggestion: Move business logic to service layer. Controller should be thin: async getApifyRunStatus(@query() query: GetApifyRunStatusDto) { const result = await this.apifyClientService.validateRunCompletion(query.runId); return result; } Service layer should contain validation logic with proper separation of concerns.

Reasoning: Violates Clean Code principles and architectural best practices. Controllers should be thin orchestrators. Business logic should be testable in isolation.

[github-actions]

⚠️ SUGGESTION

Score: 7/10
Issue: Magic string constant: Hardcoded string 'panelAgencyResult2Columns' has business significance but lacks explanation or constant definition.

Suggestion: Define named constant: export const APIFY_ACCEPTABLE_ERROR_MARKER = 'panelAgencyResult2Columns'; export const APIFY_ACCEPTABLE_ERROR_REASON = 'This error indicates a non-critical parsing issue in the agency results table that does not affect data validity'; Then use: if (firstItem.error && !firstItem.error.includes(APIFY_ACCEPTABLE_ERROR_MARKER)) { ... }

Reasoning: Violates naming conventions. Magic strings should be named constants with comments explaining their business meaning.

[github-actions]

⚠️ SUGGESTION

Score: 7/10
Issue: Unclear conditional logic with mutable state: Using mutable 'let result = true' with multiple conditional mutations is harder to reason about and violates functional programming best practices.

Suggestion: Use declarative approach with early returns: const hasDatasetItems = datasetItems.length > 0; const hasUnexpectedError = datasetItems[0]?.error && !datasetItems[0].error.includes(EXPECTED_ERROR_PATTERN); const isSuccessful = hasDatasetItems && !hasUnexpectedError; Or use early returns to flatten logic and eliminate mutable state.

Reasoning: Violates Clean Code principles. Prioritize linear code that reads top-to-bottom. Extract complex conditions to make them self-describing.

[github-actions]

⚠️ SUGGESTION

Score: 7/10
Issue: Missing input validation on request bodies: Several endpoints accept @Body() DTO objects without explicit validation decorators visible in controller.

Suggestion: Ensure ValidationPipe is configured globally or explicitly on controller: @module({ providers: [{ provide: APP_PIPE, useClass: ValidationPipe }] }) Or explicitly: async executeVentrataRegistration(@Body(ValidationPipe) request: CreateCompanyContactDto) { ... }

Reasoning: Violates security best practices. Input validation at system boundaries is critical. Only validate at system boundaries (user input, external APIs).

[github-actions]

⚠️ SUGGESTION

Score: 6/10
Issue: Overly defensive error handling: Verbose error type checking that should be handled by exception filters.

Suggestion: Use NestJS exception filter to centralize error handling logic. Controller should just throw exceptions, and filter handles response formatting consistently.

Reasoning: Violates framework best practices. NestJS exception filters centralize error handling logic and reduce code duplication.

[github-actions]

⚠️ SUGGESTION

Score: 6/10
Issue: Complex nested logic: 3 levels of nested if-statements with mixed concerns violates Clean Code's 'keep functions simple' principle.

Suggestion: Use early returns to flatten logic: if (apifyStatus !== APIFY_JOB_STATUSES.SUCCEEDED) { return { success: false, data: null, message: Retrieved run status: ${runResult.status} }; } if (!runResult.defaultDatasetId) { return { success: false, data: null, message: 'No dataset available' }; } Continue with flattened validation logic.

Reasoning: Violates code readability principles. Flat code with guard clauses is easier to read and reduces mental jumps.

[github-actions]

⚠️ SUGGESTION

Score: 6/10
Issue: Optional chaining inconsistency: Mixes traditional null checks with optional chaining, creating redundant checks.

Suggestion: Use consistent optional chaining: if (datasetItems[0]?.error && !datasetItems[0].error.includes('panelAgencyResult2Columns')) Or extract to variable: const firstItem = datasetItems[0]; if (firstItem?.error && !firstItem.error.includes('panelAgencyResult2Columns'))

Reasoning: Violates readability. If you check datasetItems[0] exists, you don't need ?. on subsequent property accesses in the same expression. Reduces cognitive load.

[github-actions]

⚠️ SUGGESTION

Score: 6/10
Issue: Re-thrown errors without context: Errors are re-thrown without wrapping context, making debugging harder. Caller doesn't know which API operation failed.

Suggestion:

catch (error: unknown) {
  this.logger.error(`Error registering agent/agency for company: ${request.companyName}`, error);
  throw new InternalServerErrorException(
    `Failed to register agent and agency. Reason: ${error instanceof Error ? error.message : 'Unknown error'}`
  );
}

Reasoning: Error messages should diagnose and provide context for upstream handlers. Improves debugging and error tracking.