feat: add setup wizard with CSV import and @wareflow/db package#10
feat: add setup wizard with CSV import and @wareflow/db package#10
Conversation
- Update electron-vite to v5.0.0 for vite 7.x compatibility - Add tailwindcss support to desktop renderer config - Add dev:web, dev:desktop, dev:all scripts at root - Configure both apps to use 127.0.0.1:3000 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create /setup route with 5-step wizard - Add FileSelection component (drag & drop) - Add Preview component (data preview table) - Add ColumnMapping component (column mapping UI) - Add Validation component (error/warning display) - Add Complete component (success screen) - Add types for import process Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![marty-action[bot]](https://avatars.githubusercontent.com/in/2883066?s=60&v=4){kind=small}
| @@ -23,13 +25,21 @@ export default defineConfig({ | |||
| } | |||
| }, | |||
| renderer: { | |||
There was a problem hiding this comment.
The root change from ../web/dist to ../web is correct for development mode, but ensure production builds still work correctly since dist is the output directory.
| return | ||
| } | ||
|
|
||
| // Simple CSV parsing for now |
There was a problem hiding this comment.
The simple CSV parsing here won't handle quoted values containing commas correctly (e.g., "Doe, John", 25). Consider using a proper CSV parser library like papaparse for robust handling.
| import { Button } from '../ui/button' | ||
|
|
||
| const MAX_FILE_SIZE = 10 * 1024 * 1024 // 10MB | ||
| const ACCEPTED_TYPES = ['.csv', '.xlsx'] |
There was a problem hiding this comment.
The component accepts .xlsx files in the UI but only performs simple CSV parsing via file.text(). XLSX files will not parse correctly. Either add xlsx parsing library or remove .xlsx from accepted types.
apps/web/src/routes/setup.tsx
Outdated
| } | ||
| }, [step]) | ||
|
|
||
| const handleImport = useCallback(() => { |
There was a problem hiding this comment.
This is a stub implementation. The TODO should be addressed before merging - actual database/dexie integration is needed for the import to work.
| private-key: ${{ secrets.MARTY_APP_PRIVATE_KEY }} | ||
|
|
||
| - name: Run Marty PR Review | ||
| uses: nesalia-inc/marty-action@1.0.0 |
There was a problem hiding this comment.
Using a third-party action from nesalia-inc/marty-action. Ensure this is a trusted action and consider pinning to a specific version tag (e.g., @v1.0.0) instead of a floating tag to prevent unexpected changes.
| }, | ||
| renderer: { | ||
| root: resolve(__dirname, '../web/dist'), | ||
| root: resolve(__dirname, '../web'), |
There was a problem hiding this comment.
This is a positive change - pointing to the source directory instead of dist allows the desktop app to use the web app's Vite dev server with HMR.
|
@@ -0,0 +1,14 @@ Review SummarySummary: This PR updates electron-vite to v5.0.0, adds TailwindCSS support to desktop, introduces dev:all scripts, and adds a setup wizard for product imports along with a PR review GitHub Actions workflow. Critical Issues:
Recommendations:
Positive Notes:
|
| @@ -1,5 +1,7 @@ | |||
| import { defineConfig, externalizeDepsPlugin } from 'electron-vite' | |||
There was a problem hiding this comment.
The externalizeDepsPlugin is correctly imported from 'electron-vite' which is the correct import path for version 5.0.0. The configuration looks compatible with electron-vite 5.x.
| } | ||
| }, | ||
| server: { | ||
| host: '127.0.0.1', |
There was a problem hiding this comment.
Both apps are configured to use port 3000. When running dev:all, both the web app and desktop renderer will try to bind to the same port, causing a conflict. Consider assigning different ports (e.g., web on 3000, desktop renderer on 3001) or using the web app's server for the desktop renderer.
SummaryThe PR successfully adds the Critical IssuesPort Conflict (apps/desktop/electron.vite.config.ts:41): Both the web app and desktop renderer are configured to use port 3000. Running
Recommendations
Positive Notes
|
- Add Geist fonts (sans + mono) - Add color-scheme and theme-color meta for dark mode - Improve accessibility: aria-label, focus-visible, keyboard handlers - Add aria-live for error messages - Replace input with proper label Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| "dev": "turbo dev", | ||
| "dev:web": "turbo dev --filter=web", | ||
| "dev:desktop": "turbo dev --filter=desktop", | ||
| "dev:all": "turbo run dev --filter=web --filter=desktop", |
There was a problem hiding this comment.
Security Concern: The dev:all script uses turbo run dev but this will run both apps in parallel without waiting for dependencies to be ready. Also, there's no explicit port conflict handling if both apps try to use the same port.
The desktop app renderer is configured to point to the web app (root: resolve(__dirname, '../web')), but the web app runs on port 3000. This could cause issues during parallel development.
| @@ -23,13 +25,21 @@ export default defineConfig({ | |||
| } | |||
| }, | |||
| renderer: { | |||
There was a problem hiding this comment.
Security Issue: The desktop renderer is sharing the same index.html from the web app. This means the desktop app will load the web app's HTML which includes the same CSP policy. However, the desktop app needs to load the actual desktop renderer entry point (/src/main.tsx), not the web app's entry. This configuration could cause the desktop app to fail to load properly or bypass security policies intended for the web app.
| ], | ||
| build: { | ||
| rollupOptions: { | ||
| input: { |
There was a problem hiding this comment.
Issue: The desktop renderer configuration uses the web app's root and index.html, but the desktop renderer should have its own entry point. Currently, both web and desktop will share the same renderer, which may not be the intended behavior for a desktop application.
Recommendation: Consider creating a separate renderer entry point for the desktop app (e.g., apps/desktop/src/renderer/) or ensure this is intentional for your architecture.
| tanstackRouter({ target: 'react', autoCodeSplitting: true }), | ||
| viteReact(), | ||
| ], | ||
| server: { |
There was a problem hiding this comment.
Port Configuration: Both web and desktop apps are configured to use port 3000. When running dev:all, there's no conflict handling. The desktop app renderer is configured to load from the web app's dev server at port 3000, but this requires the web app to start first.
Recommendation: Consider using different ports or adding a startup delay/await mechanism in the dev:all script to ensure the web app starts before the desktop app tries to connect.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SummaryThis PR updates electron-vite to v5.0.0 for Vite 7.x compatibility, adds Tailwind CSS support to the desktop renderer, and introduces new development scripts (dev:web, dev:desktop, dev:all) at the root level. It also adds a PR review GitHub Actions workflow. Critical Issues
Recommendations
Positive Notes
|
| return | ||
| } | ||
|
|
||
| const headers = lines[0].split(',').map(h => h.trim().replace(/"/g, '')) |
There was a problem hiding this comment.
The CSV parsing here is simplified and doesn't handle quoted fields containing commas (e.g., "Smith, John",30,New York). Consider using a proper CSV parsing library like papaparse for robust handling.
| import { useCallback, useState } from 'react' | ||
| import { Upload, FileSpreadsheet, AlertCircle } from 'lucide-react' | ||
|
|
||
| const MAX_FILE_SIZE = 10 * 1024 * 1024 // 10MB |
There was a problem hiding this comment.
The component accepts .xlsx files (line 5) but the implementation only parses CSV text. There's no actual .xlsx parsing logic - this will cause errors for Excel files. Either remove .xlsx from ACCEPTED_TYPES or implement proper Excel parsing.
| private-key: ${{ secrets.MARTY_APP_PRIVATE_KEY }} | ||
|
|
||
| - name: Run Marty PR Review | ||
| uses: nesalia-inc/marty-action@1.0.0 |
There was a problem hiding this comment.
For security, third-party actions should be pinned to a specific commit SHA rather than a version tag. This prevents potential supply chain attacks if the version tag is redirected. Consider using nesalia-inc/marty-action@a0b1c2d... (replace with actual commit SHA).
| const quantity = row[quantityField] | ||
| const numQuantity = parseFloat(quantity) | ||
|
|
||
| if (!quantity || quantity.trim() === '') { |
There was a problem hiding this comment.
There's a discrepancy: IMPORT_FIELDS in types/setup.ts marks quantity as required: true, but the validation logic here (line 52 comment) treats it as optional. Either update the type definition or the validation logic to be consistent.
apps/web/src/routes/setup.tsx
Outdated
| }, [step]) | ||
|
|
||
| const handleImport = useCallback(() => { | ||
| // TODO: Connect to Dexie/Backend here |
There was a problem hiding this comment.
This is a placeholder - the actual import to Dexie/backend needs to be implemented. Consider adding a tracking issue or TODO comment to ensure this gets implemented before release.
SummaryThis PR adds several enhancements: updates electron-vite to v5.0.0 for Vite 7.x compatibility, adds Tailwind CSS support to the desktop renderer, introduces dev scripts (dev:web, dev:desktop, dev:all), configures both apps to use 127.0.0.1:3000, adds a PR review GitHub Actions workflow, and implements a complete 5-step setup wizard for data import. Critical Issues
Recommendations
Positive Notes
|
- Create packages/db with schema and utilities - Add isSetupRequired, importProducts, resetApp functions - Update web app to use shared package - Connect import wizard to Dexie backend Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SummaryThis PR updates electron-vite to v5.0.0 for Vite 7.x compatibility, adds TailwindCSS support to the desktop renderer, introduces new dev scripts (dev:web, dev:desktop, dev:all), configures both apps to use 127.0.0.1:3000, and adds a PR review GitHub Actions workflow. Critical IssuesNone identified. Recommendations
Positive Notes
|
AliiiBenn
changed the title
| const errors: ValidationError[] = [] | ||
| const warnings: ValidationWarning[] = [] | ||
|
|
||
| const skuField = columnMapping.sku |
There was a problem hiding this comment.
Issue: Missing check for unmapped required fields
The validation runs but doesn't check if required fields (sku, name, quantity) are actually mapped. The component will run validation even if columnMapping.sku is undefined, leading to confusing error messages.
| let warehouse = await db.warehouses.toCollection().first() | ||
| if (!warehouse) { | ||
| const warehouseId = await db.warehouses.add({ | ||
| name: 'Main Warehouse', |
There was a problem hiding this comment.
Issue: Hardcoded warehouse name
"Main Warehouse" is hardcoded. Consider allowing users to configure the warehouse name during setup or at least making it configurable.
|
|
||
| export type ParsedData = Record<string, string> | ||
|
|
||
| export type ColumnMapping = Partial<Record<ImportField['key'], string>> |
There was a problem hiding this comment.
Issue: Type safety - ColumnMapping should not be Partial
ColumnMapping is defined as Partial<Record<...>> but required fields must be mapped. This type doesn't enforce that required fields are actually mapped, leading to potential runtime errors.
packages/db/src/utils.ts
Outdated
| updatedAt: new Date() | ||
| })) | ||
|
|
||
| await db.products.bulkAdd(products) |
There was a problem hiding this comment.
Issue: No try-catch around bulkAdd
If bulkAdd throws (e.g., due to duplicate keys or database errors), the error is not caught. Consider wrapping in try-catch for better error handling.
| return | ||
| } | ||
|
|
||
| Papa.parse(file, { |
There was a problem hiding this comment.
Issue: PapaParse runs synchronously in browser
For large files, this could block the UI. Consider using PapaParse worker mode or chunking for better performance.
|
Review: This PR looks good overall but has some issues to address. |
SummaryThis PR introduces a setup wizard for CSV product import and creates a new @wareflow/db package using Dexie. The implementation includes file selection, preview, column mapping, validation, and import completion steps. Critical Issues
Recommendations
Positive Notes
|
- Add check for required field mappings before validating data - Use bulkPut instead of bulkAdd to handle duplicates - Add try-catch with proper error messages for DB operations - Show mapping errors in validation step Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| } | ||
|
|
||
| // Get or create sector | ||
| const sectorName = data[0]?.[mapping.sector] || 'Default' |
There was a problem hiding this comment.
The sector is determined only from the first row of data (data[0]?.[mapping.sector]). All products will be imported into a single sector regardless of their sector field values in the CSV. Consider processing unique sectors from all rows.
|
|
||
| try { | ||
| // Use bulkPut to handle duplicates (update existing or add new) | ||
| await db.products.bulkPut(products) |
There was a problem hiding this comment.
Duplicate SKUs trigger a warning but the import proceeds with bulkPut, which overwrites existing records. Consider either blocking the import when duplicates exist or making this behavior explicit to users.
| @@ -0,0 +1,85 @@ | |||
| import Dexie from 'dexie' | |||
There was a problem hiding this comment.
No tests exist for this feature. Consider adding unit tests for CSV parsing, validation logic, and database import functions.
| return | ||
| } | ||
|
|
||
| const ext = file.name.substring(file.name.lastIndexOf('.')).toLowerCase() |
There was a problem hiding this comment.
Good file validation with size and type checks. Consider also validating the file content (magic bytes) in addition to extension to prevent MIME type spoofing attacks.
| <ArrowLeft className="w-4 h-4 mr-2" /> | ||
| Back | ||
| </Button> | ||
| <Button onClick={onImport} disabled={errorCount > 0 || isImporting}> |
There was a problem hiding this comment.
Consider disabling the import button when there are warnings in addition to errors, or make the overwrite behavior explicit to users to prevent unexpected data loss.
SummaryOverall, this is a well-implemented feature that adds a setup wizard with CSV import functionality and a new @wareflow/db package using Dexie for IndexedDB storage. The code follows React best practices and provides a good user experience. Critical Issues
Recommendations
Positive Notes
|
- Add gradient background - Improve progress indicator with better visual hierarchy - Enhance file selection with better icons and animations - Add help text for expected CSV columns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SummaryThis PR introduces several significant features:
Critical Issues1. Missing TypeScript Types for @wareflow/db Packagepackages/db/src/db.ts:1-67 The db package exports TypeScript types but lacks proper type definitions. The "types": "./src/index.ts"This should be generated from a proper .d.ts file or the package should export interfaces separately. 2. Bulk Import Without Transaction Supportpackages/db/src/utils.ts:70-77 The bulk import uses await db.transaction('rw', db.products, async () => {
await db.products.bulkPut(products)
})Currently, if there's a partial failure, data could be in an inconsistent state. 3. Security - No Input Sanitization on CSV Dataapps/web/src/components/setup/file-selection.tsx:30-58 The CSV parsing doesn't sanitize input data before storing. Malicious CSV files could contain scripts or other potentially harmful content. Consider sanitizing string fields. 4. Missing Error Boundary for Setup WizardThe setup wizard components don't have error boundaries. If parsing fails or an error occurs mid-flow, the entire app could crash. Recommendations1. Add TestsNo test files were found in the codebase. Add tests for:
2. Add Loading Statesapps/web/src/components/setup/validation.tsx:215-218 The import button shows a loading state, but the actual import function should also handle:
3. Improve Column Mapping Auto-Detectionapps/web/src/components/setup/column-mapping.tsx:25-52 The auto-detection logic could be improved:
4. File Size Limit Warningapps/web/src/components/setup/file-selection.tsx:19-22 Current implementation silently rejects files > 10MB. Consider showing a warning before the user selects a file. 5. Missing Database Version Migration Strategypackages/db/src/db.ts:58-64 The database schema has version 1, but there's no migration strategy for future schema changes. Add migration callbacks for version upgrades. Positive Notes
|
| "private": true, | ||
| "type": "module", | ||
| "main": "./src/index.ts", | ||
| "types": "./src/index.ts", |
There was a problem hiding this comment.
The types field should point to a proper .d.ts file rather than the source. Consider generating types with tsc --declaration or export interfaces separately for better type safety.
|
|
||
| try { | ||
| // Use bulkPut to handle duplicates (update existing or add new) | ||
| await db.products.bulkPut(products) |
There was a problem hiding this comment.
This bulk import should be wrapped in a Dexie transaction (db.transaction('rw', db.products, async () => {...})) to ensure atomicity. If there's a partial failure, data could be left in an inconsistent state.
| } | ||
|
|
||
| export function Validation({ data, columnMapping, onBack, onImport, isImporting = false, error }: ValidationProps) { | ||
| const validation = useMemo((): ValidationResult => { |
There was a problem hiding this comment.
The validation logic runs on every render due to missing data in the dependency array. This could cause performance issues with large datasets. Consider using useMemo with proper memoization or moving validation to a separate function that's called on demand.
|
|
||
| constructor() { | ||
| super('wareflow') | ||
| this.version(1).stores({ |
There was a problem hiding this comment.
There's no migration strategy for future schema changes. Add .upgrade() callbacks for version upgrades to handle schema migrations properly when the database version increments.
| sector = { id: sectorId, warehouseId: warehouse.id!, name: sectorName, createdAt: new Date(), updatedAt: new Date() } | ||
| } | ||
|
|
||
| const products: Omit<Product, 'id'>[] = data.map(row => ({ |
There was a problem hiding this comment.
No tests found in the codebase. Consider adding tests for:
- CSV parsing and validation logic (
Papa.parseresults) - Database operations (import, reset)
- Component rendering with React Testing Library
This is especially important for data validation logic which is critical for data integrity.
| }: ColumnMappingProps) { | ||
| const sampleData = useMemo(() => data.slice(0, 3), [data]) | ||
|
|
||
| const autoDetectMapping = useMemo(() => { |
There was a problem hiding this comment.
The auto-detection only examines the first row of data. Consider sampling multiple rows (e.g., first 5) to improve detection accuracy, especially if the first row might have empty values or unusual formatting.
| updatedAt: new Date() | ||
| })) | ||
|
|
||
| try { |
There was a problem hiding this comment.
Consider adding error handling for failed bulkPut operations that might fail partially. Currently if some products fail to import, the entire operation may not provide clear feedback about which specific products failed.
| const sku = row[skuField] | ||
| if (!sku || sku.trim() === '') { | ||
| errors.push({ row: rowNum, field: 'sku', message: 'SKU is required' }) | ||
| } else if (seenSkus.has(sku)) { |
There was a problem hiding this comment.
The duplicate SKU check adds a warning but the import still proceeds with bulkPut. This could lead to duplicate products in the database if the same SKU exists in the CSV. Consider either blocking duplicates or providing a clear option to handle them.
| import Dexie from 'dexie' | ||
| import { db, type Warehouse, type Sector, type Product } from './db' | ||
|
|
||
| export type ParsedData = Record<string, string> |
There was a problem hiding this comment.
This type definition duplicates ParsedData from apps/web/src/types/setup.ts. Consider exporting the type from the shared @wareflow/db package or using a single source of truth to avoid type duplication.
| } | ||
|
|
||
| // Get or create sector | ||
| const sectorName = data[0]?.[mapping.sector] || 'Default' |
There was a problem hiding this comment.
Currently, only one sector is created/retrieved based on the first row's sector value. If the CSV contains products from multiple sectors, they'll all be assigned to the same sector. Consider creating multiple sectors or allowing the user to select a default sector during import.
| errors?: string[] | ||
| } | ||
|
|
||
| export const isSetupRequired = async (): Promise<boolean> => { |
There was a problem hiding this comment.
This function checks if there are any products, but the setup wizard creates a default warehouse and sector. Consider also checking if warehouses exist to determine if setup is truly required.
SummaryThis PR introduces a comprehensive setup wizard for CSV product import and creates a new Critical Issues
Recommendations
Positive Notes
Review generated by Claude Code |
1 participant
Summary
Test plan
pnpm dev:alland verify both web and desktop apps start🤖 Generated with Claude Code