Skip to content

Conversation

@ManukMinasyan
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings January 1, 2026 16:40
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request introduces a comprehensive refactoring of the import wizard's preview functionality, adding asynchronous processing, virtual scrolling, and error filtering capabilities. The changes shift from a synchronous, sampled preview approach to a hybrid model that processes an initial batch synchronously for instant feedback, then completes the remaining rows asynchronously in the background.

Key Changes:

  • Replaces synchronous preview generation with hybrid sync/async processing using background jobs
  • Implements virtual scrolling for large datasets with on-demand row fetching via API endpoints
  • Adds error filtering UI to the review step, allowing users to view only problematic values
  • Introduces session-based import management with automatic cleanup via scheduled command

Reviewed changes

Copilot reviewed 34 out of 36 changed files in this pull request and generated 15 comments.

Show a summary per file
File Description
tests/Feature/Filament/App/Imports/ImportPreviewServiceTest.php Removed entire test suite for old preview service
tests/Feature/Filament/App/Imports/CsvReaderFactoryTest.php Removed reader caching tests and beforeEach cache clear
resources/views/filament/app/import-preview-alpine.blade.php New Alpine.js component for preview table with polling and virtual scroll
package.json & package-lock.json Added @tanstack/virtual-core dependency
composer.json & composer.lock Updated laravel/boost to v1.8
bootstrap/app.php Added hourly import:cleanup scheduled command
app/Providers/Filament/AppPanelProvider.php Registered import-preview-alpine view in render hook
app-modules/ImportWizard/src/Services/PreviewChunkService.php New service replacing ImportPreviewService with chunk-based processing
app-modules/ImportWizard/src/Services/CsvRowCounter.php Deleted - row counting moved inline
app-modules/ImportWizard/src/Services/CsvReaderFactory.php Removed caching functionality and clearCache method
app-modules/ImportWizard/src/Services/CsvAnalyzer.php Minor code style improvements
app-modules/ImportWizard/src/Services/CompanyMatcher.php Added type hints and Str import
app-modules/ImportWizard/src/Livewire/ImportWizard.php Added sessionId property and showOnlyErrors filter state
app-modules/ImportWizard/src/Livewire/ImportPreviewTable.php New isolated Livewire component for preview table
app-modules/ImportWizard/src/Livewire/Concerns/HasValueAnalysis.php Added toggleShowOnlyErrors method for error filtering
app-modules/ImportWizard/src/Livewire/Concerns/HasImportPreview.php Major refactor to hybrid sync/async preview with cache-based progress
app-modules/ImportWizard/src/Livewire/Concerns/HasCsvParsing.php Changed to session folder structure and cleanup logic
app-modules/ImportWizard/src/Jobs/StreamingImportCsv.php Removed unused ImportChunkProcessed event dispatch
app-modules/ImportWizard/src/Jobs/ProcessImportPreview.php New background job for async preview processing
app-modules/ImportWizard/src/ImportWizardServiceProvider.php Registered routes, new component, and cleanup command
app-modules/ImportWizard/src/Http/Controllers/PreviewController.php New API controller for status polling and row fetching
app-modules/ImportWizard/src/Filament/Imports/PeopleImporter.php Removed redundant try-catch wrapper around firstOrCreate
app-modules/ImportWizard/src/Filament/Imports/OpportunityImporter.php Removed redundant try-catch wrappers
app-modules/ImportWizard/src/Filament/Imports/Concerns/HasPolymorphicEntityAttachment.php Refactored entity attachment into loop-based approach
app-modules/ImportWizard/src/Filament/Imports/BaseImporter.php Updated comment reference to PreviewChunkService
app-modules/ImportWizard/src/Events/ImportChunkProcessed.php Deleted - event no longer used
app-modules/ImportWizard/src/Data/CompanyMatchResult.php Removed unused helper methods (isIdMatch, isNew, etc.)
app-modules/ImportWizard/src/Data/ColumnAnalysis.php Added paginatedErrorValues and errorIssues methods for filtering
app-modules/ImportWizard/src/Console/CleanupOrphanedImportsCommand.php New command for cleaning up old import sessions
app-modules/ImportWizard/routes/web.php New route file with preview API endpoints
app-modules/ImportWizard/resources/views/livewire/partials/step-review.blade.php Added error filter toggle button and logic
app-modules/ImportWizard/resources/views/livewire/partials/step-preview.blade.php Replaced inline preview with nested ImportPreviewTable component
app-modules/ImportWizard/resources/views/livewire/import-preview-table.blade.php New view for isolated preview table with Alpine integration
app-modules/ImportWizard/config/import-wizard.php Simplified config to only session_ttl_hours
Comments suppressed due to low confidence (4)

tests/Feature/Filament/App/Imports/CsvReaderFactoryTest.php:48

  • Tests for reader caching functionality have been removed, but the removal of the useCache parameter and clearCache method from CsvReaderFactory is not tested. Consider adding a test to verify the factory still works correctly without caching.
    app-modules/ImportWizard/src/Services/PreviewChunkService.php:128
  • When an error occurs during row processing, only the error message is included in the enriched row data. However, the original row data from the CSV is lost in the catch block. This means errored rows won't display the actual CSV column values, making it difficult for users to identify which specific data caused the error. Consider including the formatted row data along with the error information.
    app-modules/ImportWizard/resources/views/livewire/partials/step-review.blade.php:82
  • When showOnlyErrors is toggled, the reviewPage is reset to 1 (line 223 in HasValueAnalysis), but the "Showing X of Y" display at line 81 could be confusing. When errors only mode is active, it shows "Showing X of Y" where Y is errorValueCount, but when toggled back to show all, the page is reset and Y becomes uniqueCount. The transition might confuse users. Consider adding a visual indicator or label to clarify what's being shown.
                if ($showOnlyErrors && $hasColumnErrors) {
                    $values = $selectedAnalysis?->paginatedErrorValues($reviewPage, $perPage) ?? [];
                    $totalUnique = $errorValueCount;
                } else {
                    $values = $selectedAnalysis?->paginatedValues($reviewPage, $perPage) ?? [];
                    $totalUnique = $selectedAnalysis?->uniqueCount ?? 0;
                }

                $showing = min($reviewPage * $perPage, $totalUnique);
                $hasMore = $showing < $totalUnique;
            @endphp

            @if ($selectedAnalysis)
                {{-- Column Header with Stats --}}
                <div class="flex items-center justify-between px-3 py-2 border-b border-gray-200 dark:border-gray-700 bg-gray-50 dark:bg-gray-800/50">
                    <div class="flex items-center gap-3">
                        <div class="text-xs text-gray-500 dark:text-gray-400">
                            <span class="font-medium text-gray-700 dark:text-gray-300">{{ number_format($selectedAnalysis->uniqueCount) }}</span> unique values
                        </div>
                        @if ($hasColumnErrors)
                            <button
                                type="button"
                                wire:click="toggleShowOnlyErrors"
                                @class([
                                    'inline-flex items-center gap-1.5 px-2 py-1 text-xs font-medium rounded-md transition-colors',
                                    'bg-danger-100 text-danger-700 dark:bg-danger-900 dark:text-danger-300' => $showOnlyErrors,
                                    'text-gray-600 hover:bg-gray-100 dark:text-gray-400 dark:hover:bg-gray-700' => !$showOnlyErrors,
                                ])
                            >
                                <x-filament::icon icon="heroicon-m-funnel" class="h-3.5 w-3.5" />
                                {{ $showOnlyErrors ? 'Show all' : 'Errors only (' . $errorValueCount . ')' }}
                            </button>
                        @endif
                    </div>
                    <div class="text-xs text-gray-400">
                        Showing {{ number_format($showing) }} of {{ number_format($totalUnique) }}
                    </div>

tests/Feature/Filament/App/Imports/ImportPreviewServiceTest.php:1

  • The entire test file for ImportPreviewService has been removed, but the new PreviewChunkService that replaces it has no test coverage. This is a critical service that processes CSV rows and determines create/update actions. Tests should cover:
  • Chunk processing with different row ranges
  • Error handling for invalid rows
  • Value corrections
  • Company match enrichment
  • Row metadata generation

Comment on lines +1 to +91
<?php

declare(strict_types=1);

namespace Relaticle\ImportWizard\Http\Controllers;

use App\Models\User;
use Illuminate\Http\JsonResponse;
use Illuminate\Http\Request;
use Illuminate\Routing\Controller;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\Storage;
use League\Csv\Reader;
use League\Csv\Statement;

/**
* API controller for import preview operations.
*/
final class PreviewController extends Controller
{
/**
* Get the current preview processing status.
*/
public function status(string $sessionId): JsonResponse
{
$this->validateSession($sessionId);

$enrichedPath = Storage::disk('local')->path("temp-imports/{$sessionId}/enriched.csv");

return response()->json([
'status' => Cache::get("import:{$sessionId}:status", 'pending'),
'progress' => Cache::get("import:{$sessionId}:progress", [
'processed' => 0,
'creates' => 0,
'updates' => 0,
'total' => 0,
]),
'hasEnrichedFile' => file_exists($enrichedPath),
]);
}

/**
* Fetch a range of rows from the enriched CSV for virtual scroll.
*/
public function rows(Request $request, string $sessionId): JsonResponse
{
$this->validateSession($sessionId);

$enrichedPath = Storage::disk('local')->path("temp-imports/{$sessionId}/enriched.csv");

if (! file_exists($enrichedPath)) {
return response()->json(['error' => 'Session not found'], 404);
}

$start = $request->integer('start', 0);
$limit = min($request->integer('limit', 100), 500);

try {
$csv = Reader::createFromPath($enrichedPath, 'r');
$csv->setHeaderOffset(0);

$rows = iterator_to_array(
Statement::create()->offset($start)->limit($limit)->process($csv)
);

return response()->json([
'rows' => array_values($rows),
'start' => $start,
'count' => count($rows),
]);
} catch (\Throwable $e) {
report($e);

return response()->json(['error' => 'Failed to read preview data'], 500);
}
}

/**
* Validate that the session belongs to the current team.
*/
private function validateSession(string $sessionId): void
{
/** @var User|null $user */
$user = auth()->user();
$teamId = $user?->currentTeam?->getKey();

if ($teamId === null || Cache::get("import:{$sessionId}:team") !== $teamId) {
abort(404, 'Session not found');
}
}
}
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PreviewController API endpoints have no test coverage. These endpoints handle:

  • Session validation and authorization
  • Status polling for preview processing
  • Row fetching for virtual scrolling

Tests should verify authentication, authorization (team ownership), error handling, and proper data retrieval.

Copilot uses AI. Check for mistakes.
Comment on lines +41 to +50
if (Carbon::createFromTimestamp($lastModified)->lt($cutoff)) {
$sessionId = basename($dir);

Storage::disk('local')->deleteDirectory($dir);
Cache::forget("import:{$sessionId}:status");
Cache::forget("import:{$sessionId}:progress");
Cache::forget("import:{$sessionId}:team");

$deleted++;
}
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleanup command doesn't handle errors during directory deletion. If Storage::disk('local')->deleteDirectory($dir) fails at line 44, the cache keys will still be deleted (lines 45-47) and $deleted will be incremented, creating an inconsistent state. Consider wrapping the deletion logic in a try-catch block and only delete cache keys and increment the counter if directory deletion succeeds.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +57
<?php

declare(strict_types=1);

namespace Relaticle\ImportWizard\Console;

use Carbon\Carbon;
use Illuminate\Console\Command;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\Storage;

final class CleanupOrphanedImportsCommand extends Command
{
/**
* @var string
*/
protected $signature = 'import:cleanup {--hours=24 : Delete sessions older than this many hours}';

/**
* @var string
*/
protected $description = 'Clean up orphaned import session files';

public function handle(): int
{
$hours = (int) $this->option('hours');
$cutoff = now()->subHours($hours);
$deleted = 0;

$directories = Storage::disk('local')->directories('temp-imports');

foreach ($directories as $dir) {
$originalFile = "{$dir}/original.csv";

if (! Storage::disk('local')->exists($originalFile)) {
continue;
}

$lastModified = Storage::disk('local')->lastModified($originalFile);

if (Carbon::createFromTimestamp($lastModified)->lt($cutoff)) {
$sessionId = basename($dir);

Storage::disk('local')->deleteDirectory($dir);
Cache::forget("import:{$sessionId}:status");
Cache::forget("import:{$sessionId}:progress");
Cache::forget("import:{$sessionId}:team");

$deleted++;
}
}

$this->info("Deleted {$deleted} orphaned import sessions.");

return Command::SUCCESS;
}
}
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CleanupOrphanedImportsCommand has no test coverage. This command is scheduled to run hourly and handles cleanup of import sessions. Tests should verify:

  • Correct identification of orphaned sessions based on age
  • Proper deletion of files and cache keys
  • Correct count of deleted sessions
  • Edge cases (missing files, invalid directories)

Copilot uses AI. Check for mistakes.
Comment on lines +45 to +49
public function rows(Request $request, string $sessionId): JsonResponse
{
$this->validateSession($sessionId);

$enrichedPath = Storage::disk('local')->path("temp-imports/{$sessionId}/enriched.csv");
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sessionId parameter is used directly in file path construction without validation. While validateSession checks team ownership, the sessionId itself should be validated to ensure it's a valid UUID format before being used in path construction to prevent potential path traversal attacks. Add validation such as: if (!Str::isUuid($sessionId)) { abort(404); }

Copilot uses AI. Check for mistakes.
Comment on lines +55 to +63
$start = $request->integer('start', 0);
$limit = min($request->integer('limit', 100), 500);

try {
$csv = Reader::createFromPath($enrichedPath, 'r');
$csv->setHeaderOffset(0);

$rows = iterator_to_array(
Statement::create()->offset($start)->limit($limit)->process($csv)
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The start parameter from user input is passed directly to Statement::create()->offset($start) without validation for negative values. While Request::integer() returns 0 for invalid input, it's safer to explicitly ensure the start value is non-negative to prevent unexpected behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +154
<?php

declare(strict_types=1);

namespace Relaticle\ImportWizard\Jobs;

use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Queue\SerializesModels;
use Illuminate\Support\Facades\Cache;
use League\Csv\Writer;
use Relaticle\ImportWizard\Filament\Imports\BaseImporter;
use Relaticle\ImportWizard\Services\ImportRecordResolver;
use Relaticle\ImportWizard\Services\PreviewChunkService;

/**
* Background job to process remaining import preview rows.
*
* The first batch is processed synchronously for instant feedback.
* This job handles the remaining rows asynchronously.
*/
final class ProcessImportPreview implements ShouldQueue
{
use Dispatchable;
use InteractsWithQueue;
use Queueable;
use SerializesModels;

private const int CHUNK_SIZE = 500;

/**
* @param class-string<BaseImporter> $importerClass
* @param array<string, string> $columnMap
* @param array<string, mixed> $options
* @param array<string, array<string, string>> $valueCorrections
*/
public function __construct(
public string $sessionId,
public string $csvPath,
public string $enrichedPath,
public string $importerClass,
public array $columnMap,
public array $options,
public string $teamId,
public string $userId,
public int $startRow,
public int $totalRows,
public int $initialCreates,
public int $initialUpdates,
public array $valueCorrections = [],
) {
$this->onQueue('imports');
}

public function handle(PreviewChunkService $service): void
{
$processed = $this->startRow;
$creates = $this->initialCreates;
$updates = $this->initialUpdates;

// Pre-load records for fast lookups
$recordResolver = app(ImportRecordResolver::class);
$recordResolver->loadForTeam($this->teamId, $this->importerClass);

// Open enriched CSV for appending
$writer = Writer::createFromPath($this->enrichedPath, 'a');

try {
while ($processed < $this->totalRows) {
$limit = min(self::CHUNK_SIZE, $this->totalRows - $processed);

$result = $service->processChunk(
importerClass: $this->importerClass,
csvPath: $this->csvPath,
startRow: $processed,
limit: $limit,
columnMap: $this->columnMap,
options: $this->options,
teamId: $this->teamId,
userId: $this->userId,
valueCorrections: $this->valueCorrections,
recordResolver: $recordResolver,
);

// Write rows to CSV
foreach ($result['rows'] as $row) {
$writer->insertOne($service->rowToArray($row, $this->columnMap));
}

$creates += $result['creates'];
$updates += $result['updates'];
$processed += $limit;

// Update progress in cache
$this->updateProgress($processed, $creates, $updates);
}

// Mark as ready
Cache::put(
"import:{$this->sessionId}:status",
'ready',
now()->addHours($this->ttlHours())
);
} catch (\Throwable $e) {
report($e);

Cache::put(
"import:{$this->sessionId}:status",
'failed',
now()->addHours($this->ttlHours())
);

throw $e;
}
}

/**
* Update progress in cache.
*/
private function updateProgress(int $processed, int $creates, int $updates): void
{
Cache::put(
"import:{$this->sessionId}:progress",
[
'processed' => $processed,
'creates' => $creates,
'updates' => $updates,
'total' => $this->totalRows,
],
now()->addHours($this->ttlHours())
);
}

private function ttlHours(): int
{
return (int) config('import-wizard.session_ttl_hours', 24);
}

/**
* Get the tags for the job.
*
* @return array<int, string>
*/
public function tags(): array
{
return [
'import-preview',
"session:{$this->sessionId}",
"team:{$this->teamId}",
];
}
}
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new ProcessImportPreview job has no test coverage. This job handles background processing of import previews and includes critical logic for:

  • Processing chunks of rows
  • Updating progress in cache
  • Writing to enriched CSV
  • Error handling and status management

Consider adding tests for successful processing, error scenarios, and progress tracking.

Copilot uses AI. Check for mistakes.
Comment on lines +43 to +49
$model = $entity['model']::firstOrCreate(
['name' => trim((string) $name), 'team_id' => $teamId],
['creator_id' => $creatorId, 'creation_source' => CreationSource::IMPORT]
);

/** @phpstan-ignore-next-line */
$record->{$entity['relation']}()->syncWithoutDetaching([$model->id]);
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactored loop simplifies entity attachment logic, but the variable name 'model' is misleading as it's used both as a class reference (line 43) and an instance (line 49). Consider renaming the array structure to use 'modelClass' instead of 'model' for clarity, or using separate variables: $modelClass = $entity['model']; $instance = $modelClass::firstOrCreate(...);

Suggested change
$model = $entity['model']::firstOrCreate(
['name' => trim((string) $name), 'team_id' => $teamId],
['creator_id' => $creatorId, 'creation_source' => CreationSource::IMPORT]
);
/** @phpstan-ignore-next-line */
$record->{$entity['relation']}()->syncWithoutDetaching([$model->id]);
$modelClass = $entity['model'];
/** @var Model $entityModel */
$entityModel = $modelClass::firstOrCreate(
['name' => trim((string) $name), 'team_id' => $teamId],
['creator_id' => $creatorId, 'creation_source' => CreationSource::IMPORT]
);
/** @phpstan-ignore-next-line */
$record->{$entity['relation']}()->syncWithoutDetaching([$entityModel->id]);

Copilot uses AI. Check for mistakes.
Comment on lines +85 to +93
$errorValues = $this->errorIssues()->pluck('value')->all();

$filteredValues = array_filter(
$this->uniqueValues,
fn (int $count, string $value): bool => in_array($value, $errorValues, true),
ARRAY_FILTER_USE_BOTH
);

return array_slice($filteredValues, 0, $page * $perPage, preserve_keys: true);
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The paginatedErrorValues method could be inefficient for large datasets. It first extracts all error values (line 85), then filters the entire uniqueValues array (lines 87-91), and finally slices the result. For columns with many unique values but few errors, this performs unnecessary iteration. Consider optimizing by building the filtered array directly from error issues with their occurrence counts from uniqueValues, avoiding the full array filter.

Suggested change
$errorValues = $this->errorIssues()->pluck('value')->all();
$filteredValues = array_filter(
$this->uniqueValues,
fn (int $count, string $value): bool => in_array($value, $errorValues, true),
ARRAY_FILTER_USE_BOTH
);
return array_slice($filteredValues, 0, $page * $perPage, preserve_keys: true);
$valuesWithErrors = [];
foreach ($this->errorIssues() as $issue) {
$value = $issue->value;
if (array_key_exists($value, $this->uniqueValues) && ! array_key_exists($value, $valuesWithErrors)) {
$valuesWithErrors[$value] = $this->uniqueValues[$value];
}
}
return array_slice($valuesWithErrors, 0, $page * $perPage, preserve_keys: true);

Copilot uses AI. Check for mistakes.
->name('import.')
->group(function (): void {
Route::get('/{sessionId}/status', [PreviewController::class, 'status'])
->name('preview-status');
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The routes don't include CSRF protection middleware explicitly. While the 'web' middleware group typically includes CSRF, these are GET routes fetching data, so they should be safe. However, consider adding rate limiting middleware to prevent abuse of the polling endpoint (preview-status) which could be called frequently by the Alpine component every 500ms.

Suggested change
->name('preview-status');
->name('preview-status')
->middleware('throttle:300,1');

Copilot uses AI. Check for mistakes.
'failed',
now()->addHours($this->ttlHours())
);

Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the job catches an exception, it sets the status to 'failed' but doesn't store any error details (line 111). Users will see that processing failed but won't know why. Consider storing the error message in cache so it can be displayed to the user, e.g., Cache::put("import:{$this->sessionId}:error", $e->getMessage(), ...).

Suggested change
Cache::put(
"import:{$this->sessionId}:error",
$e->getMessage(),
now()->addHours($this->ttlHours())
);

Copilot uses AI. Check for mistakes.
@ManukMinasyan ManukMinasyan merged commit 88d2a45 into 3.x Jan 1, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants