-
Notifications
You must be signed in to change notification settings - Fork 0
feat: csv import page #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add StagesPreviewTable to show parsed stages data - Add SetsPreviewTable with time format validation - Parse CSV immediately on file selection to show preview - Validate time_start and time_end formats using existing timeUtils - Show validation errors inline in preview table - Display valid/invalid count badges in preview header - Highlight invalid rows with background color - Clear preview when import completes or dialog closes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove complex conflict detection and resolution system - Delete CSVImportDialog, ImportConflictResolver, conflictDetector, artistResolver, useMergeSets - Auto-detect user's timezone as default - Simplify CSVImportPage to single view without dialogs - Clean up all imports and unused code - Prepare for inline artist selection in preview table Next step: Add artist dropdowns and set matching to preview table 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Created setMatcher service for detecting matching sets by artists/edition - Fixed query to use left joins instead of inner joins for better results - Added useMatchingSetsQuery hook for react-query data loading - Refactored SetsPreviewTable into modular components: - SetPreviewRow: handles individual row rendering - ArtistSelectionCell: manages multiple artist selections - ArtistSelect: reusable select component for artist matching - StageCellWithValidation: displays stage with error messages - TimeCellWithValidation: displays time with error messages - MatchingSetCell: displays matching set information - Artist selector shows all artists with exact match highlighting (✓) - Users can select existing artists or create new ones from CSV - Updated setImporter to accept artist mappings and create artists as needed - Fixed artist creation to use added_by field instead of created_by - Wired up artist selections in CSVImportPage with proper state management 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Auto-select first matching set by default - Add dropdown to select from all sets in edition (matching + other sets) - Display sets grouped by matching/other with vote counts and stage info - Fetch stage names in set queries for better UX
…ndling - Auto-select first matching set by default in CSV import - Allow selecting from all sets in edition (matching + other sets grouped) - Display sets with vote counts and stage info for better selection - Add ImportResults component to show detailed error messages in UI - Update stage_id when matching or duplicating sets during import - Extend duplicateSetWithVotes to accept stage_id and description parameters
…arams - Remove group_id column reference (doesn't exist in votes table) - Add optional new_stage_id and new_description parameters - Use COALESCE to fall back to source set values if not provided - Add grants for both old and new function signatures
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this 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 PR introduces a dedicated CSV import page for festivals, replacing the previous dialog-based import system. The key changes include a new full-page import interface with enhanced set matching and duplication capabilities, removal of the old conflict resolution system, and improved artist mapping controls.
- Adds a new
/admin/festivals/:festivalId/:editionId/importroute with a dedicated CSV import page - Implements intelligent set matching to detect existing sets and provide options to match, duplicate, or create new
- Introduces set duplication with vote preservation via a new database function
- Refactors the import flow with better validation and preview capabilities
Reviewed Changes
Copilot reviewed 42 out of 46 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
supabase/migrations/20251112000000_add_duplicate_set_with_votes.sql |
Adds database function to duplicate sets while preserving votes, with optional stage and description parameters |
src/services/csv/setMatcher.ts |
New service for matching imported sets with existing sets based on artist names |
src/services/csv/setDuplicator.ts |
New service wrapping the database function for set duplication |
src/services/csv/timeValidator.ts |
New validation service for time and date fields in CSV imports |
src/services/csv/setSelectionValidator.ts |
Validates that multiple CSV rows don't try to match the same existing set |
src/services/csv/setImporter.ts |
Major refactor to support artist mappings and set selections for match/duplicate/create actions |
src/services/csv/csvParser.ts |
Updated to support separate date and time fields, removed conflict detection |
src/services/csv/types.ts |
Removed unused conflict-aware import types |
src/pages/admin/festivals/CSVImportPage.tsx |
New full-page CSV import interface with festival/edition selection and tabbed import |
src/pages/admin/festivals/CSVImportDialog/SetsPreviewTable.tsx |
New preview table with artist selection and set matching UI |
src/pages/admin/festivals/CSVImportDialog/* |
Multiple new components for preview tables, validation cells, and selection controls |
src/pages/admin/festivals/StageManagement.tsx |
Updated to link to new import page instead of dialog |
src/pages/admin/festivals/SetsManagement/SetManagement.tsx |
Updated to link to new import page instead of dialog |
src/hooks/queries/sets/useMatchingSetsQuery.ts |
New query hook for finding matching sets during import |
src/hooks/queries/sets/useSetsByEdition.ts |
Enhanced to include stage names |
src/lib/timeUtils.ts |
Added combineDateAndTime helper and improved error handling |
src/main.tsx |
Removed persistence layer and adjusted cache settings |
src/components/router/GlobalRoutes.tsx |
Added routes for the new CSV import page |
package.json |
Updated Supabase dependencies to newer versions |
| queryKey: ["matchingSets", { existingSets: setsQuery.data!, importedSets }], | ||
| queryFn: () => | ||
| findMatchingSets({ importedSets, existingSets: setsQuery.data! }), |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The destructuring uses non-null assertions (setsQuery.data!) in both the queryKey and queryFn, which could lead to runtime errors if setsQuery.data is undefined when the query runs. While the enabled condition checks for !!setsQuery.data, race conditions could still occur. Consider using optional chaining or a safer pattern.
| queryKey: ["matchingSets", { existingSets: setsQuery.data!, importedSets }], | |
| queryFn: () => | |
| findMatchingSets({ importedSets, existingSets: setsQuery.data! }), | |
| queryKey: ["matchingSets", { existingSets: setsQuery.data ?? [], importedSets }], | |
| queryFn: () => | |
| findMatchingSets({ importedSets, existingSets: setsQuery.data ?? [] }), |
| useEffect(() => { | ||
| const initialSetSelections = new Map<number, SetSelection>(); | ||
|
|
||
| sets.forEach((_set, index) => { | ||
| if (!matchingSetsQuery.data) return; | ||
|
|
||
| const matchingSetsForRow = matchingSetsQuery.data?.get(index) || []; | ||
| if (matchingSetsForRow.length > 0) { | ||
| initialSetSelections.set(index, { | ||
| action: "match", | ||
| matchedSetId: matchingSetsForRow[0].id, | ||
| }); | ||
| } else { | ||
| initialSetSelections.set(index, { | ||
| action: "create", | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| setSetSelections(initialSetSelections); | ||
| onSetSelectionsChange?.(initialSetSelections); | ||
| }, [sets, matchingSetsQuery.data, onSetSelectionsChange]); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The effect has onSetSelectionsChange in its dependency array (line 117), which could cause the effect to re-run unnecessarily. Similar to the artist selections effect above, this could lead to performance issues. Additionally, the effect depends on matchingSetsQuery.data but doesn't handle the case where this data changes after initial load, which could lead to stale selections.
| }, | ||
| refetchOnWindowFocus: false, | ||
| refetchOnReconnect: true, // Refetch when coming back online | ||
| staleTime: 5_000, |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The query configuration has been changed to remove offline-first support and caching strategies. The staleTime was reduced from 15 minutes to 5 seconds, and gcTime (garbage collection) was removed entirely. This could lead to more frequent network requests and reduced performance. If this change is intentional to reduce caching, consider documenting the reason or ensuring it aligns with the application's data freshness requirements.
| staleTime: 5_000, | |
| // Use a 15-minute staleTime to enable offline-first caching and reduce network requests. | |
| // Set gcTime to 30 minutes to keep unused queries in cache longer. | |
| staleTime: 15 * 60 * 1000, // 15 minutes | |
| gcTime: 30 * 60 * 1000, // 30 minutes |
| if (stageId !== undefined) { | ||
| params.new_stage_id = stageId; | ||
| } |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duplicate function accepts stageId as string | null | undefined, but when it's undefined, the parameter is omitted from the RPC call (lines 28-30). However, when stageId is an empty string "", it's included in the params, which may not be a valid UUID. Consider adding validation: if (stageId !== undefined && stageId !== "") { params.new_stage_id = stageId || null; }
| newTimeStart: utcTimeStart!, | ||
| newTimeEnd: utcTimeEnd!, |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The non-null assertion operator ! is used without checking if utcTimeStart and utcTimeEnd are actually non-null. While there are null checks above (lines 204-207), TypeScript doesn't recognize that these guarantee the values are non-null here. Consider removing the ! operators since the checks above already ensure these values exist, or refactor to make the flow clearer.
|
|
||
| setArtistSelections(initialArtistSelections); | ||
| onArtistSelectionsChange?.(initialArtistSelections); | ||
| }, [sets, artistsByName, onArtistSelectionsChange]); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The effect has onArtistSelectionsChange in its dependency array (line 94), which could cause unnecessary re-renders if the parent component doesn't memoize this callback. Since this effect initializes artist selections based on the CSV data and shouldn't re-run when the callback changes, consider using useCallback in the parent or removing this dependency and calling it separately after state updates.
| }, [sets, artistsByName, onArtistSelectionsChange]); | |
| }, [sets, artistsByName]); |
| .filter(Boolean) || [], | ||
| stage_name: set.stages?.name || null, | ||
| set_artists: undefined, // Remove junction data from final response | ||
| stages: undefined, // Remove junction data from final response |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stages property is explicitly set to undefined to remove it from the final response. However, TypeScript expects either the property to exist or not be present at all. Setting it to undefined may cause type mismatches if the FestivalSet type doesn't include stages?: .... Consider using object destructuring with rest parameters to exclude the property instead: const { stages, set_artists, ...rest } = set; return { ...rest, artists: [...], stage_name: stages?.name || null };
| stageId: stageId, | ||
| description: importedSet.description, |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stageId parameter can be an empty string (line 163), but the database function expects either a valid UUID or NULL. When stageId is an empty string, line 217 sets it as stageId || null, which correctly converts empty string to null. However, for the duplicate action (line 235), the empty string is passed directly without this conversion, which could cause database errors. Ensure consistent handling: stageId: stageId || null should be used in the duplicate call as well.
| function normalizeArtistName(name: string) { | ||
| return name | ||
| .toLowerCase() | ||
| .trim() | ||
| .replace(/[.,;!?]+$/, ""); | ||
| } |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function normalizeArtistName is defined inside the loop (line 59), which means it gets recreated for every iteration of existingSets. This is inefficient. Consider moving this function outside the loop to avoid repeated function creation.
| const { count: voteCount } = await supabase | ||
| .from("votes") | ||
| .select("*", { count: "exact", head: true }) | ||
| .eq("set_id", existingSet.id); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates an N+1 query pattern where a database query is made for every matching set to get the vote count (lines 79-82). For CSV imports with many sets, this could result in hundreds of sequential database calls. Consider using a single query to fetch all vote counts at once, or include the vote count in the initial sets query.
No description provided.