-
Notifications
You must be signed in to change notification settings - Fork 449
fix: user-facing duplicate handling workflow for bucket-based connectors (backport of #2077) #2096
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
Changes from all commits
78c6316
229a6cc
6662b29
cac63e5
5200e57
2104693
f8b54f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,12 +8,28 @@ import type { useSyncConnector } from "@/app/api/mutations/useSyncConnector"; | |
| import { useGetSettingsQuery } from "@/app/api/queries/useGetSettingsQuery"; | ||
| import { IngestSettings } from "@/components/cloud-picker/ingest-settings"; | ||
| import { getIngestChunkSettingsError } from "@/components/cloud-picker/types"; | ||
| import { DuplicateHandlingDialog } from "@/components/duplicate-handling-dialog"; | ||
| import { FileBrowserDialog } from "@/components/file-browser-dialog"; | ||
| import { Button } from "@/components/ui/button"; | ||
| import { useAuth } from "@/contexts/auth-context"; | ||
| import { useSessionIngestSettings } from "@/hooks/useSessionIngestSettings"; | ||
| import { trackProcessFailure, trackStartProcess } from "@/lib/analytics"; | ||
|
|
||
| interface BucketDuplicateFile { | ||
| id: string; | ||
| name: string; | ||
| mimeType: string; | ||
| downloadUrl?: string; | ||
| size?: number; | ||
| } | ||
|
|
||
| interface BucketDuplicateCheckResponse { | ||
| duplicate_names?: string[]; | ||
| duplicate_count?: number; | ||
| duplicate_files?: BucketDuplicateFile[]; | ||
| non_duplicate_files?: BucketDuplicateFile[]; | ||
| } | ||
|
|
||
| export interface SharedBucketViewProps { | ||
| connector: any; | ||
| buckets: Array<{ name: string; ingested_count: number }> | undefined; | ||
|
|
@@ -70,6 +86,15 @@ export function SharedBucketView({ | |
| const [browseDialogBucket, setBrowseDialogBucket] = useState<string | null>( | ||
| null, | ||
| ); | ||
| const [isCheckingDuplicates, setIsCheckingDuplicates] = useState(false); | ||
| const [duplicateDialogOpen, setDuplicateDialogOpen] = useState(false); | ||
| const [pendingDuplicates, setPendingDuplicates] = useState<{ | ||
| duplicateNames: string[]; | ||
| duplicateCount: number; | ||
| duplicateFiles: BucketDuplicateFile[]; | ||
| nonDuplicateFiles: BucketDuplicateFile[]; | ||
| } | null>(null); | ||
| const isOverwriteConfirmedRef = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| if ( | ||
|
|
@@ -111,20 +136,43 @@ export function SharedBucketView({ | |
| }); | ||
| }; | ||
|
|
||
| const ingestSelected = () => { | ||
| const chunkErr = getIngestChunkSettingsError(ingestSettings); | ||
| if (chunkErr) { | ||
| toast.error("Could not start ingest", { description: chunkErr }); | ||
| return; | ||
| } | ||
| trackStartProcess({ | ||
| const onSyncSettled = () => { | ||
| invalidate(); | ||
| }; | ||
|
|
||
| const onSyncError = (err: unknown) => { | ||
| trackProcessFailure({ | ||
| processType: "Ingestion", | ||
| process: "Document Upload", | ||
| category: "Knowledge", | ||
| source: "connector", | ||
| connector_type: connector.type, | ||
| total_buckets: selectedBuckets.size, | ||
| resultValue: err instanceof Error ? err.message : "Sync failed", | ||
| }); | ||
| toast.error(err instanceof Error ? err.message : "Sync failed"); | ||
| }; | ||
|
|
||
| const onSyncSuccess = (result: { task_ids?: string[]; message?: string }) => { | ||
| onSyncSettled(); | ||
| if (result.task_ids?.length) { | ||
| // The container path may return two tasks (new files + changed files); | ||
| // track them all. | ||
| for (const id of result.task_ids) { | ||
| addTask(id, { | ||
| connectorType: connector.type, | ||
| source: "connector", | ||
| }); | ||
| } | ||
| onDone(); | ||
| } else { | ||
| toast.info(result.message ?? "No files found in the selected buckets."); | ||
| } | ||
| }; | ||
|
|
||
| // Full bucket sync: backend classifies new/changed/unchanged itself and | ||
| // re-ingests both new and changed blobs (used both when there are no | ||
| // duplicates and when the user chooses to overwrite them). | ||
| const runBucketSync = () => { | ||
| syncMutation.mutate( | ||
| { | ||
| connectorType: connector.type, | ||
|
|
@@ -138,35 +186,129 @@ export function SharedBucketView({ | |
| : undefined, | ||
| }, | ||
| }, | ||
| { onSuccess: onSyncSuccess, onError: onSyncError }, | ||
| ); | ||
| }; | ||
|
|
||
| // Sync explicit files. `replaceDuplicates` forces an unconditional | ||
| // re-ingest (bypassing the bucket_filter path's modified-time gate) — | ||
| // used when the user confirms "Overwrite duplicates" so an already | ||
| // up-to-date file still gets re-ingested, matching what "overwrite" means | ||
| // for the OAuth connectors. | ||
| const runSelectedFilesSync = ( | ||
| files: BucketDuplicateFile[], | ||
| replaceDuplicates = false, | ||
| ) => { | ||
| syncMutation.mutate( | ||
| { | ||
| onSuccess: (result) => { | ||
| invalidate(); | ||
| if (result.task_ids?.length) { | ||
| // The container path may return two tasks (new files + changed files); | ||
| // track them all. | ||
| trackIngestTasks(result.task_ids); | ||
| onDone(); | ||
| } else { | ||
| toast.info( | ||
| result.message ?? "No files found in the selected buckets.", | ||
| ); | ||
| } | ||
| }, | ||
| onError: (err) => { | ||
| trackProcessFailure({ | ||
| processType: "Ingestion", | ||
| process: "Document Upload", | ||
| category: "Knowledge", | ||
| source: "connector", | ||
| connector_type: connector.type, | ||
| resultValue: err instanceof Error ? err.message : "Sync failed", | ||
| }); | ||
| toast.error(err instanceof Error ? err.message : "Sync failed"); | ||
| connectorType: connector.type, | ||
| body: { | ||
| connection_id: connector.connectionId!, | ||
| selected_files: files, | ||
| settings: ingestSettings, | ||
| replace_duplicates: replaceDuplicates, | ||
| shared: showSharedToggle | ||
| ? (ingestSettings.shared ?? false) | ||
| : undefined, | ||
| }, | ||
| }, | ||
| { onSuccess: onSyncSuccess, onError: onSyncError }, | ||
| ); | ||
| }; | ||
|
Comment on lines
+198
to
217
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Confirm whether ConnectorSyncBody/connector_sync ever reads a connection hint,
# and whether multiple active connections per connector_type are supported.
rg -n 'connection_id' src/api/connectors.py
rg -n -A5 'class ConnectorSyncBody' src/api/connectors.pyRepository: langflow-ai/openrag Length of output: 5067 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the backend sync body and resolution logic around connection_id.
sed -n '600,920p' src/api/connectors.py
# Inspect the two frontend call sites in context.
sed -n '170,235p' frontend/components/connectors/shared-bucket-view.tsx
sed -n '130,210p' frontend/components/file-browser-dialog.tsxRepository: langflow-ai/openrag Length of output: 16970 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the sync handler and any pydantic config that could change handling of extra fields.
sed -n '840,940p' src/api/connectors.py
rg -n "extra\s*=" src/api/connectors.py src -g'*.py'
rg -n "class Config|model_config" src/api/connectors.py src -g'*.py'Repository: langflow-ai/openrag Length of output: 4904 Preserve the connection used for duplicate checks in sync requests 📍 Affects 2 files
🤖 Prompt for AI Agents |
||
|
|
||
| const ingestSelected = async () => { | ||
| const chunkErr = getIngestChunkSettingsError(ingestSettings); | ||
| if (chunkErr) { | ||
| toast.error("Could not start ingest", { description: chunkErr }); | ||
| return; | ||
| } | ||
| trackStartProcess({ | ||
| processType: "Ingestion", | ||
| process: "Document Upload", | ||
| category: "Knowledge", | ||
| source: "connector", | ||
| connector_type: connector.type, | ||
| total_buckets: selectedBuckets.size, | ||
| }); | ||
|
|
||
| setIsCheckingDuplicates(true); | ||
| try { | ||
| const checkResponse = await fetch( | ||
| `/api/connectors/${connector.type}/check-duplicates`, | ||
| { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| connection_id: connector.connectionId, | ||
| bucket_filter: Array.from(selectedBuckets), | ||
| }), | ||
| }, | ||
| ); | ||
|
|
||
| if (!checkResponse.ok) { | ||
| throw new Error(`Duplicate check failed: ${checkResponse.statusText}`); | ||
| } | ||
|
|
||
| const checkData = | ||
| (await checkResponse.json()) as BucketDuplicateCheckResponse; | ||
| const duplicateNames = checkData.duplicate_names || []; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (1g) [Normal] Redundant full bucket listing on every "no duplicates" ingestProblem
Background Information
Potential Solution
|
||
| const duplicateCount = | ||
| typeof checkData.duplicate_count === "number" | ||
| ? checkData.duplicate_count | ||
| : duplicateNames.length; | ||
|
|
||
| if (duplicateCount === 0) { | ||
| runBucketSync(); | ||
| return; | ||
| } | ||
|
|
||
| setPendingDuplicates({ | ||
| duplicateNames, | ||
| duplicateCount, | ||
| duplicateFiles: checkData.duplicate_files || [], | ||
| nonDuplicateFiles: checkData.non_duplicate_files || [], | ||
| }); | ||
| setDuplicateDialogOpen(true); | ||
| } catch (err) { | ||
| console.error("[Bucket Sync] Duplicate check failed:", err); | ||
| // Fallback: proceed with the normal full sync (backend still handles | ||
| // new/changed reconciliation on its own). | ||
| runBucketSync(); | ||
| } finally { | ||
| setIsCheckingDuplicates(false); | ||
| } | ||
| }; | ||
|
|
||
| const handleOverwriteDuplicates = () => { | ||
| if (!pendingDuplicates) return; | ||
| isOverwriteConfirmedRef.current = true; | ||
| const { duplicateFiles, nonDuplicateFiles } = pendingDuplicates; | ||
| runSelectedFilesSync([...duplicateFiles, ...nonDuplicateFiles], true); | ||
| setPendingDuplicates(null); | ||
| }; | ||
|
|
||
| const handleDuplicateDialogOpenChange = (open: boolean) => { | ||
| if (!open && pendingDuplicates) { | ||
| if (isOverwriteConfirmedRef.current) { | ||
| // Overwrite already submitted in handleOverwriteDuplicates; this close | ||
| // event fires immediately after and would otherwise re-enter the | ||
| // "skip duplicates" branch. | ||
| isOverwriteConfirmedRef.current = false; | ||
| } else { | ||
| const { nonDuplicateFiles, duplicateCount } = pendingDuplicates; | ||
| if (nonDuplicateFiles.length > 0) { | ||
| runSelectedFilesSync(nonDuplicateFiles); | ||
| } else { | ||
| toast.info( | ||
| `All ${duplicateCount} file(s) already exist and were skipped. Nothing was synced.`, | ||
| ); | ||
| } | ||
| } | ||
| setPendingDuplicates(null); | ||
| } | ||
| setDuplicateDialogOpen(open); | ||
| }; | ||
|
|
||
| return ( | ||
| <> | ||
| <div className="mb-8 flex gap-2 items-center"> | ||
|
|
@@ -330,14 +472,20 @@ export function SharedBucketView({ | |
| <Button | ||
| className="bg-foreground text-background hover:bg-foreground/90 font-semibold" | ||
| onClick={ingestSelected} | ||
| disabled={syncMutation.isPending || selectedBuckets.size === 0} | ||
| loading={syncMutation.isPending} | ||
| disabled={ | ||
| syncMutation.isPending || | ||
| isCheckingDuplicates || | ||
| selectedBuckets.size === 0 | ||
| } | ||
| loading={syncMutation.isPending || isCheckingDuplicates} | ||
| > | ||
| {syncMutation.isPending | ||
| ? "Ingesting…" | ||
| : selectedBuckets.size > 0 | ||
| ? `Ingest ${selectedBuckets.size} Bucket${selectedBuckets.size !== 1 ? "s" : ""}` | ||
| : "Select Buckets to Ingest"} | ||
| : isCheckingDuplicates | ||
| ? "Checking…" | ||
| : selectedBuckets.size > 0 | ||
| ? `Ingest ${selectedBuckets.size} Bucket${selectedBuckets.size !== 1 ? "s" : ""}` | ||
| : "Select Buckets to Ingest"} | ||
| </Button> | ||
| </div> | ||
| </div> | ||
|
|
@@ -366,6 +514,15 @@ export function SharedBucketView({ | |
| ingestSettings={ingestSettings} | ||
| /> | ||
| )} | ||
|
|
||
| <DuplicateHandlingDialog | ||
| open={duplicateDialogOpen} | ||
| onOpenChange={handleDuplicateDialogOpenChange} | ||
| onOverwrite={handleOverwriteDuplicates} | ||
| isLoading={syncMutation.isPending} | ||
| duplicateNames={pendingDuplicates?.duplicateNames} | ||
| duplicateCount={pendingDuplicates?.duplicateCount} | ||
| /> | ||
| </> | ||
| ); | ||
| } | ||
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.
(1h) [Minor] Fragile ref-based workaround for dialog close/overwrite race, with no test coverage
Problem
isOverwriteConfirmedRefexists to suppresshandleDuplicateDialogOpenChange's "skip" branch from re-firing whenDuplicateHandlingDialog's overwrite handler callsonOpenChange(false)right afteronOverwrite(). It works, but depends onpendingDuplicatesstill being non-null in a stale closure at the momentonOpenChange(false)fires synchronously afteronOverwrite(), before React flushes the state update clearing it. Any future change toDuplicateHandlingDialog's call order could silently break this, and no frontend/e2e test exercises either the overwrite or skip path.Background Information
isOverwriteConfirmedRefis still present and used identically in fix: user-facing duplicate handling workflow for bucket-based connectors (backport of #2077) #2096'sshared-bucket-view.tsx:97,284,292,296.Potential Solution
pendingDuplicatesin one state update, and have the dialog's own Cancel/backdrop-close path call a distinctonSkipcallback rather than overloadingonOpenChange. Add a Playwright test covering: no-duplicates auto-sync, duplicates→overwrite, duplicates→skip, duplicates→dismiss-without-choice.