Add flat file import process#22
Open
sprucely wants to merge 11 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds an end-to-end CSV/XLSX flat-file dataset import feature: an ChangesFlat-File Import Feature
Sequence Diagram(s)sequenceDiagram
participant Browser as FlatFileImportPanel
participant RQ as React Query
participant API as ImportsController
participant Service as FlatFileImportService
participant SQL as SQL Server
Browser->>RQ: mount — fetch /api/imports/recent
RQ->>API: GET /api/imports/recent
API->>SQL: Query ImportLogs (latest per dataset)
SQL-->>API: RecentImportLog[]
API-->>RQ: ImportDatasetSummaryResponse[]
RQ-->>Browser: Render per-dataset summary cards
Browser->>API: POST /api/imports/{dataset} (FormData)
API->>Service: ImportAsync(datasetId, file, user)
Service->>SQL: Parse, validate, BulkCopy `#FlatFileImportRows`
Service->>SQL: StagingValidation + DELETE/INSERT target table
Service->>SQL: INSERT ImportLog (Succeeded)
Service-->>API: ImportSucceeded
API-->>Browser: 200 OK ImportSuccessResponse
alt Validation errors
Service-->>API: ImportValidationFailed
API-->>Browser: 400 ImportValidationResponse
Browser->>Browser: Render ValidationResults with highlighted cells
end
Browser->>API: GET /api/imports/{id} (on card click)
API->>SQL: Load ImportLog by id
SQL-->>API: ImportLog with ErrorPayload JSON
API-->>Browser: ImportLogDetailResponse with validation history
Browser->>Browser: Render HistoricalImportDetails DataTable
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
server.core/Data/AppDbContext.cs (1)
19-20: 💤 Low valueTable naming convention inconsistency.
ImportLoguses a singular table name whileUsersis plural. Consider standardizing on plural table names for consistency (e.g.,ImportLogs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.core/Data/AppDbContext.cs` around lines 19 - 20, The ImportLog entity uses a singular table name "ImportLog" while the User entity uses a plural table name "Users", creating an inconsistency in naming conventions. In the modelBuilder configuration, change the ImportLog entity's ToTable call to use the plural form "ImportLogs" instead of "ImportLog" to align with the plural naming convention used throughout the database schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/components/FlatFileImportPanel.tsx`:
- Around line 211-232: The onError handler in FlatFileImportPanel.tsx has two
issues that need fixing. First, the queryClient.invalidateQueries call that
refreshes recent import summaries is only executed in the validation error path
but should apply to all failure scenarios, so move it outside of the if block to
ensure recent imports always refresh after any error. Second, the fallback
validation state object uses the component state variable dataset instead of
variables.dataset, which can cause the wrong dataset to display if the user
changes their selection while a request is in flight; replace the component
state dataset reference with variables.dataset in the fallback validation
object.
In `@server.core/Domain/ImportLog.cs`:
- Line 40: The ErrorPayload property in the ImportLog class is declared as
nvarchar(max) without size constraints, while other string fields like Dataset,
Filename, Name, and Email all use explicit Truncate() calls with defined limits.
This allows validation payloads from SerializeValidationLogPayload() to grow
unbounded and degrade SQL Server performance. Add a [MaxLength] attribute to the
ErrorPayload property to enforce a reasonable size limit (such as 65535 or
1048576 for 1MB), consistent with the truncation pattern used for other string
fields in the class.
In `@server/Controllers/ImportsController.cs`:
- Around line 19-24: The query in ImportsController is applying a global
Take(datasetIds.Count * 10) limit before grouping by dataset, which causes
datasets with fewer recent logs to be dropped entirely if other datasets
dominate the results. Refactor the LINQ query to group by dataset (using GroupBy
on log.Dataset) after ordering by CompletedAt, then apply the Take(10) limit per
dataset group rather than globally. This ensures each dataset in datasetIds gets
its own recent import logs regardless of how many logs other datasets have. The
Select should then project from the grouped results into RecentImportLog.
In `@server/Import/FlatFileImportService.cs`:
- Around line 167-238: The ParseWorkbook method lacks exception handling around
the workbook file operations (the XLWorkbook initialization and worksheet
access), while the parallel ParseCsv method includes proper exception handling
for malformed files. Wrap the workbook opening and initial worksheet reading
operations in a try-catch block, and when any exception occurs, return a
FlatFileParseResult containing an ImportFileError describing the parsing
failure, rather than allowing the exception to propagate. This ensures malformed
.xlsx files are treated as validation failures that return structured error
results instead of unhandled exceptions.
In `@server/Import/ImportDatasetDefinition.cs`:
- Around line 42-53: The loop building the columnsByNormalizedHeader dictionary
silently overwrites entries when multiple ImportColumn instances have source
headers that normalize to the same key, causing incorrect column mapping. Before
assigning to the columnsByNormalizedHeader dictionary in the nested loop (where
sourceHeader is iterated), check if the normalized key already exists. If it
does, throw an appropriate exception with details about which columns have the
conflicting headers to ensure collisions are detected and reported during
construction rather than causing silent failures at runtime.
---
Nitpick comments:
In `@server.core/Data/AppDbContext.cs`:
- Around line 19-20: The ImportLog entity uses a singular table name "ImportLog"
while the User entity uses a plural table name "Users", creating an
inconsistency in naming conventions. In the modelBuilder configuration, change
the ImportLog entity's ToTable call to use the plural form "ImportLogs" instead
of "ImportLog" to align with the plural naming convention used throughout the
database schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76d7bfb4-cfdc-4b64-9622-e9bea38564de
📒 Files selected for processing (20)
client/src/components/FlatFileImportPanel.tsxclient/src/routes/(authenticated)/workflow.$stageId.tsxclient/src/shared/dataTable.tsxclient/src/test/components/FlatFileImportPanel.test.tsxclient/src/test/routes/(authenticated)/workflow.test.tsxserver.core/Data/AppDbContext.csserver.core/Domain/ImportLog.csserver.core/Migrations/20260609182232_AddImportLog.Designer.csserver.core/Migrations/20260609182232_AddImportLog.csserver.core/Migrations/AppDbContextModelSnapshot.csserver/Controllers/ImportsController.csserver/Import/FlatFileImportRegistry.csserver/Import/FlatFileImportService.csserver/Import/ImportDatasetDefinition.csserver/Models/Imports/ImportResponses.csserver/Program.csserver/server.csprojtests/server.tests/Data/AppDbContextTests.cstests/server.tests/Import/FlatFileImportServiceTests.cstests/server.tests/server.tests.csproj
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/server.tests/Controllers/ImportsControllerTests.cs (1)
14-67: ⚡ Quick winAdd a same-timestamp tie-break assertion for
/recent.Line 14 currently validates latest-by-time, but not the new
Idtie-break path. Add twoall-projectslogs with identicalCompletedAtand assert the higherIdentry wins.Proposed test extension
[Fact] public async Task Recent_returns_latest_import_for_each_dataset() { await using var db = TestDbContextFactory.CreateInMemory(); var completedAt = DateTimeOffset.Parse("2026-06-17T12:00:00Z"); @@ db.ImportLogs.Add(new ImportLog { Dataset = "active-projects", Filename = "active-projects.csv", @@ }); + + // Same timestamp tie-break should pick the higher Id + db.ImportLogs.Add(new ImportLog + { + Dataset = "all-projects", + Filename = "all-projects-tie-a.csv", + Status = "Succeeded", + AttemptedRows = 999, + RowsImported = 999, + StartedAt = completedAt.AddHours(1).AddSeconds(-30), + CompletedAt = completedAt.AddHours(1), + }); + db.ImportLogs.Add(new ImportLog + { + Dataset = "all-projects", + Filename = "all-projects-tie-b.csv", + Status = "Succeeded", + AttemptedRows = 1000, + RowsImported = 1000, + StartedAt = completedAt.AddHours(1).AddSeconds(-20), + CompletedAt = completedAt.AddHours(1), + }); await db.SaveChangesAsync(); @@ - allProjects.LastImport!.Filename.Should().Be("all-projects-29.csv"); + allProjects.LastImport!.Filename.Should().Be("all-projects-tie-b.csv");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server.tests/Controllers/ImportsControllerTests.cs` around lines 14 - 67, The test method Recent_returns_latest_import_for_each_dataset() currently validates latest-import-by-time behavior but does not test the tie-break logic when two imports have the same CompletedAt timestamp. Add two ImportLog entries for the "all-projects" dataset with identical CompletedAt values before the SaveChangesAsync() call, ensuring they will have different Id values. Then add an assertion to verify that the ImportLog entry with the higher Id is the one returned in the allProjects.LastImport result, confirming that the Id tie-break logic works correctly when timestamps are equal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.core/Domain/User.cs`:
- Around line 8-9: Remove the unnecessary Index attributes that are decorating
the Email and Name properties in the User class. The [Index(nameof(Email))] and
[Index(nameof(Name))] attributes should be deleted since they add storage and
maintenance overhead without providing query performance benefits, as the only
User entity query in the codebase filters exclusively by EntraId, not by Email
or Name.
---
Nitpick comments:
In `@tests/server.tests/Controllers/ImportsControllerTests.cs`:
- Around line 14-67: The test method
Recent_returns_latest_import_for_each_dataset() currently validates
latest-import-by-time behavior but does not test the tie-break logic when two
imports have the same CompletedAt timestamp. Add two ImportLog entries for the
"all-projects" dataset with identical CompletedAt values before the
SaveChangesAsync() call, ensuring they will have different Id values. Then add
an assertion to verify that the ImportLog entry with the higher Id is the one
returned in the allProjects.LastImport result, confirming that the Id tie-break
logic works correctly when timestamps are equal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3adb1bbc-7fb4-4ab6-afe1-12fd375568f8
📒 Files selected for processing (13)
client/src/components/FlatFileImportPanel.tsxclient/src/test/components/FlatFileImportPanel.test.tsxserver.core/Domain/ImportLog.csserver.core/Domain/User.csserver.core/Migrations/20260617161705_AddIndexes.Designer.csserver.core/Migrations/20260617161705_AddIndexes.csserver.core/Migrations/AppDbContextModelSnapshot.csserver/Controllers/ImportsController.csserver/Import/FlatFileImportService.csserver/Import/ImportDatasetDefinition.cstests/server.tests/Controllers/ImportsControllerTests.cstests/server.tests/Data/AppDbContextTests.cstests/server.tests/Import/FlatFileImportServiceTests.cs
✅ Files skipped from review due to trivial changes (3)
- server.core/Migrations/20260617161705_AddIndexes.cs
- server.core/Migrations/AppDbContextModelSnapshot.cs
- server.core/Migrations/20260617161705_AddIndexes.Designer.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- server.core/Domain/ImportLog.cs
- server/Import/ImportDatasetDefinition.cs
- tests/server.tests/Data/AppDbContextTests.cs
- client/src/test/components/FlatFileImportPanel.test.tsx
- client/src/components/FlatFileImportPanel.tsx
- server/Import/FlatFileImportService.cs
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
1 participant
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.
Closes #7
Summary by CodeRabbit
Release Notes