diff --git a/backend/database/schema.sql b/backend/database/schema.sql index b4cc5d0..1adeccd 100644 --- a/backend/database/schema.sql +++ b/backend/database/schema.sql @@ -526,4 +526,71 @@ CREATE POLICY "canvas_elements_service_role" ON canvas_elements -- ============================================================ -- Enable Realtime on canvas_datasets so clients receive live dataset-linked events -ALTER PUBLICATION supabase_realtime ADD TABLE canvas_datasets; \ No newline at end of file +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 + FROM pg_publication_tables + WHERE pubname = 'supabase_realtime' + AND schemaname = 'public' + AND tablename = 'canvas_datasets' + ) THEN + ALTER PUBLICATION supabase_realtime ADD TABLE canvas_datasets; + END IF; +END; +$$; + +-- ============================================================ +-- unlink_dataset_from_canvas: atomic unlink + conditional delete +-- ============================================================ +-- Removes a dataset from a specific canvas, then hard-deletes the datasets row +-- only when no other canvases still reference it and the caller owns that row +-- (or the row is anonymous/dev-mode data). Both operations happen inside a +-- single PL/pgSQL transaction, eliminating the TOCTOU race that exists when the +-- two steps are performed as separate round-trips from the client. +-- +-- SECURITY DEFINER is required so that: +-- 1. The function can enforce canvas edit-access before unlinking. +-- 2. The function can perform the final cleanup DELETE after explicitly +-- checking ownership and remaining canvas references. +DROP FUNCTION IF EXISTS unlink_dataset_from_canvas(UUID, UUID); +CREATE OR REPLACE FUNCTION unlink_dataset_from_canvas( + p_dataset_id UUID, + p_canvas_id UUID +) RETURNS void +LANGUAGE plpgsql +SECURITY DEFINER +SET search_path = public, pg_catalog +AS $$ +BEGIN + IF NOT user_has_canvas_edit(p_canvas_id) THEN + RAISE EXCEPTION 'Not authorized to modify this canvas' USING ERRCODE = '42501'; + END IF; + + -- Step 1: remove the specific canvas link. + DELETE FROM canvas_datasets + WHERE dataset_id = p_dataset_id + AND canvas_id = p_canvas_id; + + -- If the requested link did not exist, stop here. This prevents callers from + -- passing an arbitrary unlinked dataset_id and hard-deleting an orphaned row. + IF NOT FOUND THEN + RETURN; + END IF; + + -- Step 2: delete the dataset row only when no other canvas still references it + -- and the caller is allowed to delete the dataset itself. + -- The NOT EXISTS subquery and the DELETE are evaluated atomically within this + -- transaction, preventing a concurrent linkDatasetToCanvas from racing past the + -- check before the delete fires. + DELETE FROM datasets + WHERE id = p_dataset_id + AND (user_id = auth.uid() OR user_id IS NULL) + AND NOT EXISTS ( + SELECT 1 FROM canvas_datasets WHERE dataset_id = p_dataset_id + ); +END; +$$; + +REVOKE EXECUTE ON FUNCTION unlink_dataset_from_canvas(UUID, UUID) FROM PUBLIC; +GRANT EXECUTE ON FUNCTION unlink_dataset_from_canvas(UUID, UUID) TO authenticated; diff --git a/frontend/hooks/useDatasetManager.ts b/frontend/hooks/useDatasetManager.ts index bfea3c0..8aa1ac5 100644 --- a/frontend/hooks/useDatasetManager.ts +++ b/frontend/hooks/useDatasetManager.ts @@ -2,7 +2,7 @@ import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { useCallback, useEffect } from 'react'; import Papa from 'papaparse'; import { isSupabaseConfigured, supabase } from '@/lib/supabase/client'; -import { Tables, TablesInsert } from '@/lib/supabase/types'; +import { Tables } from '@/lib/supabase/types'; import { Dataset } from '@/components/AutoVizAgent'; import { useAuthContext } from '@/components/providers/AuthProvider'; import { DatasetService, DatasetMetadata, ProcessingStatus } from '@/lib/services/datasetService'; @@ -126,28 +126,33 @@ export function useDatasetManager(options: DatasetManagerOptions = {}) { } try { - // Check if a dataset with this filename already exists to prevent duplicates - // Use canvas-scoped datasets when on a canvas so collaborators see each other's uploads + // Check if this exact completed dataset already exists to prevent duplicates. + // Use canvas-scoped datasets when on a canvas so collaborators see each other's uploads. const existingDatasets = canvasId ? await DatasetService.getCanvasDatasets(canvasId) : await DatasetService.getUserDatasets(userId); - const duplicateExists = existingDatasets.some(d => d.filename === file.name); - if (duplicateExists) { + const existingDuplicate = existingDatasets.find( + d => d.filename === file.name && + d.file_size === file.size && + d.processing_status === 'completed' + ); + if (existingDuplicate) { // Clear in-progress flag before early return so the same file can be re-uploaded later if (typeof window !== 'undefined') { delete (window as any)[inProgressKey]; } - // Don't throw error, just return existing dataset to prevent UI issues - const existing = existingDatasets.find(d => d.filename === file.name)!; - const metadata = (existing.metadata as unknown as DatasetMetadata) || {} as DatasetMetadata; + // Resolve progress/status callbacks so callers don't hang in a loading state + onStatusChange?.('completed'); + onProgress?.(100); + const metadata = (existingDuplicate.metadata as unknown as DatasetMetadata) || {} as DatasetMetadata; return { - id: existing.id, + id: existingDuplicate.id, name: file.name.replace(/\.[^/.]+$/, ''), type: 'csv', columns: metadata.columns ?? 0, rows: metadata.rows ?? 0, - size: formatFileSize(existing.file_size), - lastModified: formatDate(existing.updated_at), + size: formatFileSize(existingDuplicate.file_size), + lastModified: formatDate(existingDuplicate.updated_at), dataTypes: metadata.dataTypes ?? { numerical: 0, categorical: 0, @@ -156,10 +161,69 @@ export function useDatasetManager(options: DatasetManagerOptions = {}) { }, preview: metadata.preview ?? [], data: Array.isArray(metadata.sample_data) ? metadata.sample_data : [], - processingStatus: existing.processing_status as ProcessingStatus + processingStatus: existingDuplicate.processing_status as ProcessingStatus }; } + // When on a canvas with an authenticated user, check if this file already exists globally + // (uploaded to a different canvas). Match on both filename AND file size to avoid silently + // returning the wrong dataset when the user re-uses a filename for different data. + // Only reuse a dataset that has completed processing — failed/pending ones should be + // re-created so the user isn't stuck with a broken import. + // Skip this check in dev mode (userId null): getUserDatasets(null) returns all rows + // where user_id IS NULL, which could include datasets created by other anonymous + // sessions on the same instance. Without a user identity there is no safe way to + // determine ownership, so always create a fresh row instead. + if (canvasId && userId) { + // Wrap in try/catch so a transient fetch failure here degrades gracefully + // (skip the global dedup, proceed to upload) rather than aborting the entire + // upload with a confusing error message. + let globalDuplicate: Awaited> = null; + try { + globalDuplicate = await DatasetService.getCompletedUserDatasetByFile( + userId, + file.name, + file.size + ); + } catch (dedupeError) { + console.error('useDatasetManager: global dedup check failed, proceeding with upload:', dedupeError); + } + if (globalDuplicate) { + if (typeof window !== 'undefined') { + delete (window as any)[inProgressKey]; + } + try { + await DatasetService.linkDatasetToCanvas(globalDuplicate.id, canvasId); + } catch (linkError) { + onStatusChange?.('failed'); + onProgress?.(0); + throw linkError; + } + // Resolve progress/status callbacks so callers don't hang in a loading state + onStatusChange?.('completed'); + onProgress?.(100); + const metadata = (globalDuplicate.metadata as unknown as DatasetMetadata) || {} as DatasetMetadata; + return { + id: globalDuplicate.id, + name: file.name.replace(/\.[^/.]+$/, ''), + type: 'csv', + columns: metadata.columns ?? 0, + rows: metadata.rows ?? 0, + size: formatFileSize(globalDuplicate.file_size), + lastModified: formatDate(globalDuplicate.updated_at), + dataTypes: metadata.dataTypes ?? { + numerical: 0, + categorical: 0, + temporal: 0, + geographic: 0 + }, + preview: metadata.preview ?? [], + data: Array.isArray(metadata.sample_data) ? metadata.sample_data : [], + processingStatus: globalDuplicate.processing_status as ProcessingStatus + }; + } + } + // Step 1: Create dataset with pending status onStatusChange?.('pending'); onProgress?.(10); @@ -309,10 +373,43 @@ export function useDatasetManager(options: DatasetManagerOptions = {}) { } const userId = user?.id || null; + const dataSize = new Blob([JSON.stringify(rawData)]).size; + + // Canvas-scoped dedup: if this exact completed dataset is already linked + // to the current canvas, return it rather than re-running analysis and creating a + // duplicate datasets row. Requiring 'completed' prevents returning a broken import + // (failed/pending) when the user re-uploads to fix a previous failure. + if (canvasId && filename) { + try { + const canvasDatasets = await DatasetService.getCanvasDatasets(canvasId); + const existing = canvasDatasets.find( + d => d.filename === filename && + d.file_size === dataSize && + d.processing_status === 'completed' + ); + if (existing) { + const metadata = (existing.metadata as unknown as DatasetMetadata) || {} as DatasetMetadata; + return { + id: existing.id, + name: filename.replace(/\.[^/.]+$/, ''), + type: 'csv', + columns: metadata.columns ?? 0, + rows: metadata.rows ?? 0, + size: formatFileSize(existing.file_size), + lastModified: formatDate(existing.updated_at), + dataTypes: metadata.dataTypes ?? { numerical: 0, categorical: 0, temporal: 0, geographic: 0 }, + preview: metadata.preview ?? [], + data: Array.isArray(metadata.sample_data) ? metadata.sample_data : [], + processingStatus: existing.processing_status as ProcessingStatus, + }; + } + } catch (dedupeError) { + console.error('useDatasetManager: legacy canvas dedup check failed, proceeding with create:', dedupeError); + } + } // Analyze the data const dataAnalysis = analyzeData(rawData); - const dataSize = new Blob([JSON.stringify(rawData)]).size; // Create dataset with completed status since we have all the data const dbDataset = await DatasetService.createDataset({ @@ -398,7 +495,13 @@ export function useDatasetManager(options: DatasetManagerOptions = {}) { const removeDatasetMutation = useMutation({ mutationFn: async (datasetId: string) => { const userId = user?.id || null; - await DatasetService.deleteDataset(datasetId, userId); + // Authenticated canvas users unlink from this canvas only. Dev/anonymous mode + // falls back to user-scoped delete because the unlink RPC requires auth.uid(). + if (canvasId && userId) { + await DatasetService.removeDatasetFromCanvas(datasetId, canvasId); + } else { + await DatasetService.deleteDataset(datasetId, userId); + } return datasetId; }, onSuccess: (deletedId) => { @@ -459,7 +562,13 @@ export function useDatasetManager(options: DatasetManagerOptions = {}) { queryClient.invalidateQueries({ queryKey: ['datasets', 'canvas', canvasId] }); } ) - .subscribe(); + .subscribe((status, err) => { + if (status === 'CHANNEL_ERROR' || status === 'TIMED_OUT') { + // Don't throw — the 30 s polling fallback (refetchInterval) handles live updates. + // Log so the failure is observable without requiring a Supabase dashboard check. + console.error(`canvas_datasets realtime subscription failed (${status}):`, err); + } + }); return () => { supabase.removeChannel(channel); }; }, [canvasId, queryClient]); diff --git a/frontend/lib/services/datasetService.ts b/frontend/lib/services/datasetService.ts index b468958..0e03563 100644 --- a/frontend/lib/services/datasetService.ts +++ b/frontend/lib/services/datasetService.ts @@ -1,5 +1,6 @@ import { supabase, isSupabaseConfigured } from '@/lib/supabase/client'; import { Tables, TablesInsert, TablesUpdate } from '@/lib/supabase/types'; +import type { PostgrestError } from '@supabase/supabase-js'; export type ProcessingStatus = 'pending' | 'processing' | 'completed' | 'failed'; @@ -11,6 +12,11 @@ interface CanvasDatasetRow { added_at: string; } +type CompletedDatasetMatch = Pick< + Tables<'datasets'>, + 'id' | 'filename' | 'file_size' | 'updated_at' | 'metadata' | 'processing_status' +>; + export interface DatasetMetadata { columns: number; rows: number; @@ -37,10 +43,26 @@ export interface DatasetMetadata { }; } +type PostgrestLikeError = { + message: string; + code?: string; + details?: string | null; + hint?: string | null; +}; + +function toPostgrestError(error: PostgrestLikeError): Error & PostgrestError { + return Object.assign(new Error(error.message), { + name: 'PostgrestError', + code: error.code ?? 'UNKNOWN_ERROR', + details: error.details ?? '', + hint: error.hint ?? '', + }); +} + export class DatasetService { /** - * Create a new dataset with pending status (supports null userId for dev mode) - * Includes duplicate prevention by checking for existing datasets with the same filename + * Create a new dataset with pending status (supports null userId for dev mode). + * Callers are responsible for deduplication before calling this method. */ static async createDataset(params: { userId: string | null; @@ -51,30 +73,6 @@ export class DatasetService { }): Promise> { const { userId, filename, fileSize, fileType, initialMetadata = {} } = params; - // First, check if a dataset with this filename already exists for this user - let existingQuery = supabase - .from('datasets') - .select('id, filename') - .eq('filename', filename); - - if (userId === null) { - existingQuery = existingQuery.is('user_id', null); - } else { - existingQuery = existingQuery.eq('user_id', userId); - } - - const { data: existing, error: checkError } = await existingQuery.maybeSingle(); - - // Only throw if we actually found a duplicate (ignore query errors for now) - if (existing && !checkError) { - console.log('Dataset with filename already exists:', filename, 'ID:', existing.id); - throw new Error(`Dataset with filename "${filename}" already exists`); - } - - if (checkError) { - console.warn('Warning: Could not check for duplicates:', checkError.message); - } - const datasetInsert: TablesInsert<'datasets'> = { user_id: userId, filename, @@ -119,43 +117,51 @@ export class DatasetService { processing_status: status, }; - // Update metadata with processing info — always read-and-merge to avoid clobbering file_info + // Update metadata with processing info when available. If the metadata read fails, + // still update processing_status so rows do not get stuck in an old state. if (status === 'processing') { - const { data: currentDataset } = await supabase + const { data: currentDataset, error: fetchError } = await supabase .from('datasets') .select('metadata') .eq('id', datasetId) .single(); - const currentMetadata = (currentDataset?.metadata as unknown as DatasetMetadata) || {}; - updateData.metadata = { - ...currentMetadata, - processing_info: { - ...currentMetadata.processing_info, - started_at: new Date().toISOString(), - }, - }; + if (fetchError) { + console.error('updateProcessingStatus: failed to read metadata before marking processing; updating status only:', fetchError); + } else { + const currentMetadata = (currentDataset?.metadata as unknown as DatasetMetadata) || {}; + updateData.metadata = { + ...currentMetadata, + processing_info: { + ...currentMetadata.processing_info, + started_at: new Date().toISOString(), + }, + }; + } } else if (status === 'completed' || status === 'failed') { // Get current metadata to preserve existing data - const { data: currentDataset } = await supabase + const { data: currentDataset, error: fetchError } = await supabase .from('datasets') .select('metadata') .eq('id', datasetId) .single(); - - const currentMetadata = (currentDataset?.metadata as unknown as DatasetMetadata) || {}; - const processingInfo = currentMetadata.processing_info || {}; - - updateData.metadata = { - ...currentMetadata, - processing_info: { - ...processingInfo, - completed_at: new Date().toISOString(), - ...(processingInfo.started_at && { - processing_duration_ms: Date.now() - new Date(processingInfo.started_at).getTime(), - }), - ...(errorMessage && { error_message: errorMessage }), - }, - }; + if (fetchError) { + console.error(`updateProcessingStatus: failed to read metadata before marking ${status}; updating status only:`, fetchError); + } else { + const currentMetadata = (currentDataset?.metadata as unknown as DatasetMetadata) || {}; + const processingInfo = currentMetadata.processing_info || {}; + + updateData.metadata = { + ...currentMetadata, + processing_info: { + ...processingInfo, + completed_at: new Date().toISOString(), + ...(processingInfo.started_at && { + processing_duration_ms: Date.now() - new Date(processingInfo.started_at).getTime(), + }), + ...(errorMessage && { error_message: errorMessage }), + }, + }; + } } const { error } = await supabase @@ -278,6 +284,33 @@ export class DatasetService { return data || []; } + /** + * Find a completed user-owned dataset by exact file identity. + */ + static async getCompletedUserDatasetByFile( + userId: string, + filename: string, + fileSize: number + ): Promise { + const { data, error } = await supabase + .from('datasets') + .select('id, filename, file_size, updated_at, metadata, processing_status') + .eq('user_id', userId) + .eq('filename', filename) + .eq('file_size', fileSize) + .eq('processing_status', 'completed') + .order('updated_at', { ascending: false }) + .limit(1) + .returns(); + + if (error) { + console.error('Failed to fetch completed user dataset by file:', error); + throw toPostgrestError(error); + } + + return data?.[0] ?? null; + } + /** * Get all datasets linked to a canvas (visible to both owner and collaborators) */ @@ -299,7 +332,7 @@ export class DatasetService { if (linkError) { console.error('Failed to fetch canvas_datasets links:', linkError); - throw linkError; + throw toPostgrestError(linkError); } const datasetIds = (links || []).map((r) => r.dataset_id).filter(Boolean); @@ -337,12 +370,51 @@ export class DatasetService { if (error && error.code !== '23505') { // 23505 = unique_violation (already linked) console.error('Failed to link dataset to canvas:', error); - throw error; + throw toPostgrestError(error); + } + } + + /** + * Unlink a dataset from a specific canvas without deleting the dataset itself. + */ + static async unlinkDatasetFromCanvas(datasetId: string, canvasId: string): Promise { + const { error } = await (supabase + .from('canvas_datasets' as any) + .delete() + .eq('dataset_id', datasetId) + .eq('canvas_id', canvasId)) as unknown as { error: { message: string; code: string } | null }; + + if (error) { + console.error('Failed to unlink dataset from canvas:', error); + throw toPostgrestError(error); } } /** - * Delete dataset and all related data (supports null userId for dev mode) + * Remove a dataset from a canvas. If no other canvases reference the datasets row, + * hard-deletes it too. The unlink and conditional delete are performed atomically + * inside a single Postgres transaction via the unlink_dataset_from_canvas RPC, + * preventing the TOCTOU race that would exist across two separate round-trips. + * + * Use this instead of deleteDataset when operating from a canvas context. + */ + static async removeDatasetFromCanvas(datasetId: string, canvasId: string): Promise { + const { error } = await supabase.rpc('unlink_dataset_from_canvas' as any, { + p_dataset_id: datasetId, + p_canvas_id: canvasId, + }); + + if (error) { + console.error('Failed to remove dataset from canvas:', error); + throw toPostgrestError(error); + } + } + + /** + * Delete a datasets row and all related data (supports null userId for dev mode). + * Prefer removeDatasetFromCanvas when operating from a canvas context — it unlinks + * from the specific canvas and only hard-deletes the datasets row when no other + * canvases still reference it (canvas_datasets rows cascade on datasets DELETE). */ static async deleteDataset(datasetId: string, userId: string | null): Promise { let query = supabase @@ -451,4 +523,4 @@ export class DatasetService { console.log('Dataset metadata updated:', datasetId); } -} \ No newline at end of file +} diff --git a/frontend/lib/services/errorHandler.ts b/frontend/lib/services/errorHandler.ts index a8bf0cc..8ab599e 100644 --- a/frontend/lib/services/errorHandler.ts +++ b/frontend/lib/services/errorHandler.ts @@ -10,6 +10,15 @@ export interface DatabaseError { retryAfter?: number; // seconds } +function isPostgrestLikeError(error: unknown): error is PostgrestError { + if (typeof error !== 'object' || error === null) { + return false; + } + + const maybeError = error as { code?: unknown; message?: unknown }; + return typeof maybeError.code === 'string' && typeof maybeError.message === 'string'; +} + export class DatabaseErrorHandler { /** * Convert Supabase/PostgreSQL errors to user-friendly error objects @@ -329,7 +338,7 @@ export class DatabaseErrorHandler { */ export function useErrorHandler() { const handleError = (error: unknown): DatabaseError => { - if (error instanceof Error) { + if (error instanceof Error || isPostgrestLikeError(error)) { return DatabaseErrorHandler.handleDatabaseError(error); } @@ -355,4 +364,4 @@ export function useErrorHandler() { formatError, getRetryStrategy, }; -} \ No newline at end of file +}