From d998299373ac670301ca73dc39e972b1ac71ca40 Mon Sep 17 00:00:00 2001 From: wadii Date: Fri, 14 Aug 2026 16:27:10 +0200 Subject: [PATCH 1/5] feat: wire CSV segments to the cohorts API --- frontend/common/services/useCohort.ts | 82 ++++++++++++++++ frontend/common/types/requests.ts | 21 ++++ frontend/common/types/responses.ts | 34 +++++++ frontend/common/utils/__tests__/csv.test.ts | 25 ++++- frontend/common/utils/csv.ts | 13 ++- .../web/components/CsvUpload/CsvUpload.tsx | 23 ++++- .../modals/ConfirmRemoveSegment.tsx | 26 ++++- .../CreateSegmentFromCsv.tsx | 95 +++++++++++++++++-- .../segments/SegmentRow/SegmentRow.tsx | 56 +++++++++-- .../SegmentRow/components/SegmentAction.tsx | 2 + 10 files changed, 352 insertions(+), 25 deletions(-) create mode 100644 frontend/common/services/useCohort.ts diff --git a/frontend/common/services/useCohort.ts b/frontend/common/services/useCohort.ts new file mode 100644 index 000000000000..022a0268ee7b --- /dev/null +++ b/frontend/common/services/useCohort.ts @@ -0,0 +1,82 @@ +import { Res } from 'common/types/responses' +import { Req } from 'common/types/requests' +import { service } from 'common/service' +import toFormData from 'common/utils/toFormData' + +export const cohortService = service + .enhanceEndpoints({ addTagTypes: ['Cohort', 'Segment'] }) + .injectEndpoints({ + endpoints: (builder) => ({ + createCohort: builder.mutation({ + invalidatesTags: (q, e, arg) => [ + { id: 'LIST', type: 'Cohort' }, + { id: `LIST${arg.projectId}`, type: 'Segment' }, + ], + query: (query) => ({ + body: { + description: query.description, + metadata: query.metadata, + name: query.name, + }, + method: 'POST', + url: `environments/${query.environmentApiKey}/cohorts/`, + }), + }), + deleteCohort: builder.mutation({ + invalidatesTags: (q, e, arg) => [ + { id: 'LIST', type: 'Cohort' }, + { id: `LIST${arg.projectId}`, type: 'Segment' }, + ], + query: (query) => ({ + method: 'DELETE', + url: `environments/${query.environmentApiKey}/cohorts/${query.cohortId}/`, + }), + }), + syncCohortCsv: builder.mutation< + Res['cohortCsvSync'], + Req['syncCohortCsv'] + >({ + invalidatesTags: (q, e, arg) => [ + { id: arg.cohortId, type: 'Cohort' }, + { id: `LIST${arg.projectId}`, type: 'Segment' }, + ], + queryFn: async (query, baseQueryApi, extraOptions, baseQuery) => { + // projectId only feeds tag invalidation; keep it out of the form data. + const { cohortId, environmentApiKey, projectId: _, ...rest } = query + const formData = toFormData({ ...rest }) + const { data, error } = await baseQuery({ + body: formData, + method: 'POST', + url: `environments/${environmentApiKey}/cohorts/${cohortId}/sync-csv/`, + }) + return { data, error } as { + data: Res['cohortCsvSync'] + error: never + } + }, + }), + // END OF ENDPOINTS + }), + }) + +export async function deleteCohort( + store: any, + data: Req['deleteCohort'], + options?: Parameters[1], +) { + return store.dispatch( + cohortService.endpoints.deleteCohort.initiate(data, options), + ) +} + +export const { + useCreateCohortMutation, + useDeleteCohortMutation, + useSyncCohortCsvMutation, + // END OF EXPORTS +} = cohortService + +/* Usage examples: +const [createCohort, { isLoading, data, isSuccess }] = useCreateCohortMutation() +const [syncCohortCsv, { isLoading }] = useSyncCohortCsvMutation() +*/ diff --git a/frontend/common/types/requests.ts b/frontend/common/types/requests.ts index 8f4ae03ca9e3..a603c549352f 100644 --- a/frontend/common/types/requests.ts +++ b/frontend/common/types/requests.ts @@ -5,6 +5,7 @@ import { FeatureStateValue, ImportStrategy, Approval, + Metadata, MultivariateOption, SAMLConfiguration, Segment, @@ -173,6 +174,26 @@ export type Req = { projectId: number segment: Omit } + createCohort: { + environmentApiKey: string + projectId: number + name: string + description?: string + metadata?: Metadata[] + } + deleteCohort: { + environmentApiKey: string + cohortId: number + projectId: number + } + syncCohortCsv: { + environmentApiKey: string + cohortId: number + projectId: number + file: File + identifier_column?: number + has_header?: boolean + } cloneSegment: { projectId: number segmentId: number diff --git a/frontend/common/types/responses.ts b/frontend/common/types/responses.ts index 5eec1303ede8..0e732dd22cc1 100644 --- a/frontend/common/types/responses.ts +++ b/frontend/common/types/responses.ts @@ -174,6 +174,14 @@ export type SegmentMembersResponse = PagedResponse & { // Pass as `cursor` to fetch the next page; null when there are no more rows. next_cursor: string | null } +export type SegmentCohort = { + id: number + environment: number + source_type: 'csv' + version: number + deletion_requested_at: string | null +} + export type Segment = { id: number rules: SegmentRule[] @@ -184,6 +192,7 @@ export type Segment = { feature?: number metadata: Metadata[] | [] membership_counts?: SegmentMembership[] + cohort?: SegmentCohort | null } export type ProjectChangeRequest = Omit< ChangeRequest, @@ -995,6 +1004,29 @@ export type Metadata = { field_value: string } +export type Cohort = { + id: number + uuid: string + name: string + description: string | null + segment: number + source_type: 'csv' + version: number + created_at: string +} + +export type CohortCsvSyncResult = { + version: number + added: number + removed: number + unchanged: number + ignored: { + empty: number + duplicates: number + too_long: number + } +} + export type MetadataFieldModelField = { id: number content_type: number @@ -1324,6 +1356,8 @@ export type TrustRelationship = { export type Res = { segments: PagedResponse segment: Segment + cohort: Cohort + cohortCsvSync: CohortCsvSyncResult segmentMembers: SegmentMembersResponse auditLogs: PagedResponse organisationLicence: {} diff --git a/frontend/common/utils/__tests__/csv.test.ts b/frontend/common/utils/__tests__/csv.test.ts index 1d6e0664b7c1..ca17f4e500f9 100644 --- a/frontend/common/utils/__tests__/csv.test.ts +++ b/frontend/common/utils/__tests__/csv.test.ts @@ -1,4 +1,9 @@ -import { extractIdentifiers, parseCsvText, toParsedCsv } from 'common/utils/csv' +import { + extractIdentifiers, + parseCsvText, + toCsvColumn, + toParsedCsv, +} from 'common/utils/csv' describe('parseCsvText', () => { const cases: [string, string, string[][]][] = [ @@ -79,3 +84,21 @@ describe('extractIdentifiers', () => { }) }) }) + +describe('toCsvColumn', () => { + test.each([ + ['plain values', ['a', 'b'], 'a\nb'], + ['comma quoted', ['Doe, Jane', 'b'], '"Doe, Jane"\nb'], + ['quote escaped', ['say "hi"'], '"say ""hi"""'], + ['newline quoted', ['line1\nline2'], '"line1\nline2"'], + ])('%s', (_, values, expected) => { + expect(toCsvColumn(values)).toEqual(expected) + }) + + test('round-trips through parseCsvText', () => { + const values = ['plain', 'Doe, Jane', 'say "hi"', 'multi\nline'] + expect(parseCsvText(toCsvColumn(values)).map((row) => row[0])).toEqual( + values, + ) + }) +}) diff --git a/frontend/common/utils/csv.ts b/frontend/common/utils/csv.ts index e99824e4c448..8c211539ea1b 100644 --- a/frontend/common/utils/csv.ts +++ b/frontend/common/utils/csv.ts @@ -58,7 +58,10 @@ export function toParsedCsv( if (!rawRows.length) { return { columns: [], rows: [] } } - const columnCount = Math.max(...rawRows.map((cells) => cells.length)) + const columnCount = rawRows.reduce( + (max, cells) => Math.max(max, cells.length), + 0, + ) if (hasHeaders) { const [header, ...rows] = rawRows return { @@ -96,3 +99,11 @@ export function extractIdentifiers( } return { duplicateCount, emptyCount, identifiers } } + +export function toCsvColumn(values: string[]): string { + return values + .map((value) => + /[",\r\n]/.test(value) ? `"${value.replace(/"/g, '""')}"` : value, + ) + .join('\n') +} diff --git a/frontend/web/components/CsvUpload/CsvUpload.tsx b/frontend/web/components/CsvUpload/CsvUpload.tsx index f1e93ebbb93d..9fa6668ab757 100644 --- a/frontend/web/components/CsvUpload/CsvUpload.tsx +++ b/frontend/web/components/CsvUpload/CsvUpload.tsx @@ -9,6 +9,7 @@ import './CsvUpload.scss' export type CsvUploadType = { value: File | null + maxSizeBytes?: number rowCount?: number onChange: (file: File, text: string) => void } @@ -20,7 +21,12 @@ const formatFileSize = (bytes: number) => { return `${(bytes / 1024).toFixed(1)} KB` } -const CsvUpload: FC = ({ onChange, rowCount, value }) => { +const CsvUpload: FC = ({ + maxSizeBytes, + onChange, + rowCount, + value, +}) => { const [error, setError] = useState('') const onDrop = useCallback( @@ -46,12 +52,17 @@ const CsvUpload: FC = ({ onChange, rowCount, value }) => { accept: { 'text/csv': ['.csv'], }, + maxSize: maxSizeBytes, multiple: false, noClick: true, noKeyboard: true, onDrop, - onDropRejected: () => { - setError('Please select a CSV file') + onDropRejected: (rejections) => { + setError( + rejections[0]?.errors?.[0]?.code === 'file-too-large' && maxSizeBytes + ? `Please select a file smaller than ${formatFileSize(maxSizeBytes)}` + : 'Please select a CSV file', + ) }, }) @@ -91,7 +102,11 @@ const CsvUpload: FC = ({ onChange, rowCount, value }) => { )} - {!!error && } + {!!error && ( +
+ +
+ )} ) } diff --git a/frontend/web/components/modals/ConfirmRemoveSegment.tsx b/frontend/web/components/modals/ConfirmRemoveSegment.tsx index d1de098dff78..b9f6eec4420e 100644 --- a/frontend/web/components/modals/ConfirmRemoveSegment.tsx +++ b/frontend/web/components/modals/ConfirmRemoveSegment.tsx @@ -1,11 +1,13 @@ import React, { FC, FormEvent, useState } from 'react' -import { Segment } from 'common/types/responses' +import { Environment, Segment, SegmentCohort } from 'common/types/responses' import ProjectProvider from 'common/providers/ProjectProvider' import InputGroup from 'components/base/forms/InputGroup' import Utils from 'common/utils/utils' import Button from 'components/base/forms/Button' import ModalHR from './ModalHR' import { deleteSegment } from 'common/services/useSegment' +import { deleteCohort } from 'common/services/useCohort' +import { getEnvironments } from 'common/services/useEnvironment' import { getStore } from 'common/store' type ConfirmRemoveSegmentType = { @@ -17,9 +19,29 @@ export const handleRemoveSegment = ( segment: Segment, onComplete?: () => void, ) => { + // Cohort-managed segments must be deleted via their cohort; the segment + // endpoint rejects them. + const removeCohort = async (cohort: SegmentCohort) => { + const { data: environments } = await getEnvironments(getStore(), { + projectId: Number(projectId), + }) + const environmentApiKey = environments?.results?.find( + (environment: Environment) => environment.id === cohort.environment, + )?.api_key + if (!environmentApiKey) { + throw new Error('Cohort environment not found') + } + return deleteCohort(getStore(), { + cohortId: cohort.id, + environmentApiKey, + projectId: Number(projectId), + }) + } const removeSegmentCallback = async () => { try { - const res = await deleteSegment(getStore(), { id: segment.id, projectId }) + const res = segment.cohort + ? await removeCohort(segment.cohort) + : await deleteSegment(getStore(), { id: segment.id, projectId }) if (res.error) throw new Error(res.error) toast(
diff --git a/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx b/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx index 9d19fa1861e5..a4d0e60076d0 100644 --- a/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx +++ b/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx @@ -3,8 +3,18 @@ import classNames from 'classnames' import Constants from 'common/constants' import Format from 'common/utils/format' import Utils from 'common/utils/utils' -import { extractIdentifiers, parseCsvText, toParsedCsv } from 'common/utils/csv' +import { + extractIdentifiers, + parseCsvText, + toCsvColumn, + toParsedCsv, +} from 'common/utils/csv' import { useGetSupportedContentTypeQuery } from 'common/services/useSupportedContentType' +import { + useCreateCohortMutation, + useSyncCohortCsvMutation, +} from 'common/services/useCohort' +import { Metadata } from 'common/types/responses' import AccountStore from 'common/stores/account-store' import { colorIconSuccess } from 'common/theme/tokens' import Button from 'components/base/forms/Button' @@ -21,6 +31,8 @@ import Tabs from 'components/navigation/TabMenu/Tabs' import './CreateSegmentFromCsv.scss' const PREVIEW_ROW_COUNT = 5 +// Mirrors the API's COHORT_CSV_MAX_FILE_SIZE_BYTES. +const MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024 type CreateSegmentFromCsvType = { projectId: number | string @@ -35,6 +47,14 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { const [hasHeaders, setHasHeaders] = useState(true) const [selectedColumn, setSelectedColumn] = useState(null) const [tab, setTab] = useState(0) + const [metadata, setMetadata] = useState([]) + const [createdCohortId, setCreatedCohortId] = useState(null) + + const [createCohort, { error: createError, isLoading: isCreating }] = + useCreateCohortMutation() + const [syncCohortCsv, { error: syncError, isLoading: isSyncing }] = + useSyncCohortCsvMutation() + const isSaving = isCreating || isSyncing const metadataEnable = Utils.getPlansPermission('METADATA') const { data: supportedContentTypes } = useGetSupportedContentTypeQuery({ @@ -60,9 +80,25 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { [parsed.rows, columnIndex], ) + // Only the identifier column leaves the browser. + const csvColumn = useMemo( + () => (extraction ? toCsvColumn(extraction.identifiers) : ''), + [extraction], + ) + // Quoting can expand values, so the generated upload needs its own check. + const isUploadTooLarge = useMemo( + () => new Blob([csvColumn]).size > MAX_FILE_SIZE_BYTES, + [csvColumn], + ) + const isBlocked = !!extraction && !extraction.identifiers.length const canSubmit = - !!name && !!environmentId && !!file && !!extraction && !isBlocked + !!name && + !!environmentId && + !!file && + !!extraction && + !isBlocked && + !isUploadTooLarge const onFile = (newFile: File, text: string) => { setFile(newFile) @@ -70,9 +106,43 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { setSelectedColumn(null) } - const save = (e: FormEvent) => { + const save = async (e: FormEvent) => { e.preventDefault() - // TODO: submit to the cohorts API once the creation endpoint exists + if (!canSubmit || !extraction) { + return + } + try { + // Keep the created cohort across a failed sync so retrying only syncs. + let cohortId = createdCohortId + if (cohortId === null) { + const cohort = await createCohort({ + description: description || undefined, + environmentApiKey: environmentId, + metadata, + name, + projectId: Number(projectId), + }).unwrap() + cohortId = cohort.id + setCreatedCohortId(cohortId) + } + const result = await syncCohortCsv({ + cohortId, + environmentApiKey: environmentId, + file: new File([csvColumn], 'identifiers.csv', { type: 'text/csv' }), + has_header: false, + projectId: Number(projectId), + }).unwrap() + toast( + `Segment created with ${result.added} ${ + result.added === 1 ? 'identity' : 'identities' + }`, + 'success', + 10000, + ) + closeModal() + } catch { + // Errors surface via the mutation error states below. + } } const columnName = columnIndex === null ? '' : parsed.columns[columnIndex] @@ -126,7 +196,6 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { setEnvironmentId(`${value}`)} @@ -139,6 +208,7 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => {
@@ -250,11 +320,21 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { error={`No valid identifiers found in "${columnName}". Choose a different column or check your file.`} /> )} + {isUploadTooLarge && ( + + )} )} + {!!(createError || syncError) && ( + + )}
-
@@ -281,6 +361,7 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { projectId={Number(projectId)} entityContentType={segmentContentType.id} entity={segmentContentType.model} + onChange={(m) => setMetadata(m as Metadata[])} /> } /> diff --git a/frontend/web/components/segments/SegmentRow/SegmentRow.tsx b/frontend/web/components/segments/SegmentRow/SegmentRow.tsx index 758d27e6f662..0a3486aa646d 100644 --- a/frontend/web/components/segments/SegmentRow/SegmentRow.tsx +++ b/frontend/web/components/segments/SegmentRow/SegmentRow.tsx @@ -1,4 +1,5 @@ import { FC } from 'react' +import classNames from 'classnames' import { useHistory } from 'react-router-dom' import { useHasPermission } from 'common/providers/Permission' @@ -6,8 +7,10 @@ import { useHasPermission } from 'common/providers/Permission' import { Segment } from 'common/types/responses' import SegmentAction from './components/SegmentAction' import { SegmentMembershipTotalBadge } from 'components/segments/SegmentMembershipBadge' +import Chip from 'components/base/Chip' import ConfirmCloneSegment from 'components/modals/ConfirmCloneSegment' import { useCloneSegmentMutation } from 'common/services/useSegment' +import { useGetEnvironmentsQuery } from 'common/services/useEnvironment' import { handleRemoveSegment } from 'components/modals/ConfirmRemoveSegment' import { ProjectPermission } from 'common/types/permissions.types' @@ -19,7 +22,16 @@ interface SegmentRowProps { const SegmentRow: FC = ({ index, projectId, segment }) => { const history = useHistory() - const { description, feature, id, name } = segment + const { cohort, description, feature, id, name } = segment + + const { data: environments } = useGetEnvironmentsQuery( + { projectId: Number(projectId) }, + { skip: !cohort }, + ) + const cohortEnvironment = environments?.results?.find( + (environment) => environment.id === cohort?.environment, + ) + const isPendingDeletion = !!cohort?.deletion_requested_at const { permission: manageSegmentsPermission } = useHasPermission({ id: projectId, @@ -65,11 +77,18 @@ const SegmentRow: FC = ({ index, projectId, segment }) => { } return ( - + history.push( `${document.location.pathname.replace(/\/$/, '')}/${id}`, @@ -82,6 +101,21 @@ const SegmentRow: FC = ({ index, projectId, segment }) => { {feature && (
Feature-Specific
)} + {!!cohort && ( + + {cohort.source_type.toUpperCase()} + + )} + {!!cohort && !!cohortEnvironment && ( + + {cohortEnvironment.name} + + )} + {isPendingDeletion && ( + + Deleting + + )} @@ -91,13 +125,15 @@ const SegmentRow: FC = ({ index, projectId, segment }) => {
- + {!isPendingDeletion && ( + + )}
) diff --git a/frontend/web/components/segments/SegmentRow/components/SegmentAction.tsx b/frontend/web/components/segments/SegmentRow/components/SegmentAction.tsx index 15e60d87da5d..d5c894b30672 100644 --- a/frontend/web/components/segments/SegmentRow/components/SegmentAction.tsx +++ b/frontend/web/components/segments/SegmentRow/components/SegmentAction.tsx @@ -52,6 +52,7 @@ const SegmentAction: FC = ({ icon={} label='Clone Segment' handleActionClick={() => { + setIsOpen(false) onClone() }} entity='segment' @@ -64,6 +65,7 @@ const SegmentAction: FC = ({ icon={} label='Remove Segment' handleActionClick={() => { + setIsOpen(false) onRemove() }} entity='segment' From 5889d04be513d64a80860bb58f10b9501aa8ee5f Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 25 Aug 2026 10:50:28 +0200 Subject: [PATCH 2/5] fix: use cohort summary environment fields, cap identifier bytes client-side, cover the two-step submit --- frontend/common/types/responses.ts | 2 + frontend/common/utils/__tests__/csv.test.ts | 18 ++++++ frontend/common/utils/csv.ts | 11 +++- .../modals/ConfirmRemoveSegment.tsx | 29 +++------- .../CreateSegmentFromCsv.tsx | 55 ++++++++++++------- .../__tests__/submitCohortCsv.test.ts | 55 +++++++++++++++++++ .../CreateSegmentFromCsv/submitCohortCsv.ts | 24 ++++++++ .../segments/SegmentRow/SegmentRow.tsx | 12 +--- 8 files changed, 153 insertions(+), 53 deletions(-) create mode 100644 frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts create mode 100644 frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts diff --git a/frontend/common/types/responses.ts b/frontend/common/types/responses.ts index 0e732dd22cc1..799b3da75e66 100644 --- a/frontend/common/types/responses.ts +++ b/frontend/common/types/responses.ts @@ -177,6 +177,8 @@ export type SegmentMembersResponse = PagedResponse & { export type SegmentCohort = { id: number environment: number + environment_api_key: string + environment_name: string source_type: 'csv' version: number deletion_requested_at: string | null diff --git a/frontend/common/utils/__tests__/csv.test.ts b/frontend/common/utils/__tests__/csv.test.ts index ca17f4e500f9..0f49a1439d06 100644 --- a/frontend/common/utils/__tests__/csv.test.ts +++ b/frontend/common/utils/__tests__/csv.test.ts @@ -1,5 +1,6 @@ import { extractIdentifiers, + MAX_IDENTIFIER_BYTES, parseCsvText, toCsvColumn, toParsedCsv, @@ -73,6 +74,7 @@ describe('extractIdentifiers', () => { duplicateCount: 2, emptyCount: 2, identifiers: ['a', 'b'], + tooLongCount: 0, }) }) @@ -81,6 +83,22 @@ describe('extractIdentifiers', () => { duplicateCount: 0, emptyCount: 1, identifiers: ['y'], + tooLongCount: 0, + }) + }) + + test('identifiers over the UTF-8 byte limit are dropped', () => { + // 'é' is 2 UTF-8 bytes, so 513 of them exceed 1024 bytes in 513 chars. + const rows = [ + ['a'.repeat(MAX_IDENTIFIER_BYTES)], + ['a'.repeat(MAX_IDENTIFIER_BYTES + 1)], + ['é'.repeat(513)], + ] + expect(extractIdentifiers(rows, 0)).toEqual({ + duplicateCount: 0, + emptyCount: 0, + identifiers: ['a'.repeat(MAX_IDENTIFIER_BYTES)], + tooLongCount: 2, }) }) }) diff --git a/frontend/common/utils/csv.ts b/frontend/common/utils/csv.ts index 8c211539ea1b..4abc3f50eb26 100644 --- a/frontend/common/utils/csv.ts +++ b/frontend/common/utils/csv.ts @@ -7,8 +7,13 @@ export type ExtractedIdentifiers = { duplicateCount: number emptyCount: number identifiers: string[] + tooLongCount: number } +// Mirrors the API's COHORT_IDENTIFIER_MAX_BYTES (Edge identifiers are +// DynamoDB sort keys, capped at 1024 bytes). +export const MAX_IDENTIFIER_BYTES = 1024 + export function parseCsvText(text: string): string[][] { const rows: string[][] = [] let row: string[] = [] @@ -84,12 +89,16 @@ export function extractIdentifiers( ): ExtractedIdentifiers { const seen = new Set() const identifiers: string[] = [] + const encoder = new TextEncoder() let emptyCount = 0 let duplicateCount = 0 + let tooLongCount = 0 for (const cells of rows) { const value = (cells[columnIndex] ?? '').trim() if (!value) { emptyCount++ + } else if (encoder.encode(value).length > MAX_IDENTIFIER_BYTES) { + tooLongCount++ } else if (seen.has(value)) { duplicateCount++ } else { @@ -97,7 +106,7 @@ export function extractIdentifiers( identifiers.push(value) } } - return { duplicateCount, emptyCount, identifiers } + return { duplicateCount, emptyCount, identifiers, tooLongCount } } export function toCsvColumn(values: string[]): string { diff --git a/frontend/web/components/modals/ConfirmRemoveSegment.tsx b/frontend/web/components/modals/ConfirmRemoveSegment.tsx index b9f6eec4420e..377be3f045a4 100644 --- a/frontend/web/components/modals/ConfirmRemoveSegment.tsx +++ b/frontend/web/components/modals/ConfirmRemoveSegment.tsx @@ -1,5 +1,5 @@ import React, { FC, FormEvent, useState } from 'react' -import { Environment, Segment, SegmentCohort } from 'common/types/responses' +import { Segment } from 'common/types/responses' import ProjectProvider from 'common/providers/ProjectProvider' import InputGroup from 'components/base/forms/InputGroup' import Utils from 'common/utils/utils' @@ -7,7 +7,6 @@ import Button from 'components/base/forms/Button' import ModalHR from './ModalHR' import { deleteSegment } from 'common/services/useSegment' import { deleteCohort } from 'common/services/useCohort' -import { getEnvironments } from 'common/services/useEnvironment' import { getStore } from 'common/store' type ConfirmRemoveSegmentType = { @@ -19,28 +18,16 @@ export const handleRemoveSegment = ( segment: Segment, onComplete?: () => void, ) => { - // Cohort-managed segments must be deleted via their cohort; the segment - // endpoint rejects them. - const removeCohort = async (cohort: SegmentCohort) => { - const { data: environments } = await getEnvironments(getStore(), { - projectId: Number(projectId), - }) - const environmentApiKey = environments?.results?.find( - (environment: Environment) => environment.id === cohort.environment, - )?.api_key - if (!environmentApiKey) { - throw new Error('Cohort environment not found') - } - return deleteCohort(getStore(), { - cohortId: cohort.id, - environmentApiKey, - projectId: Number(projectId), - }) - } const removeSegmentCallback = async () => { try { + // Cohort-managed segments must be deleted via their cohort; the segment + // endpoint rejects them. const res = segment.cohort - ? await removeCohort(segment.cohort) + ? await deleteCohort(getStore(), { + cohortId: segment.cohort.id, + environmentApiKey: segment.cohort.environment_api_key, + projectId: Number(projectId), + }) : await deleteSegment(getStore(), { id: segment.id, projectId }) if (res.error) throw new Error(res.error) toast( diff --git a/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx b/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx index a4d0e60076d0..61cd1e197d29 100644 --- a/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx +++ b/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx @@ -5,6 +5,7 @@ import Format from 'common/utils/format' import Utils from 'common/utils/utils' import { extractIdentifiers, + MAX_IDENTIFIER_BYTES, parseCsvText, toCsvColumn, toParsedCsv, @@ -28,6 +29,7 @@ import Icon from 'components/icons/Icon' import AddMetadataToEntity from 'components/metadata/AddMetadataToEntity' import TabItem from 'components/navigation/TabMenu/TabItem' import Tabs from 'components/navigation/TabMenu/Tabs' +import { submitCohortCsv } from './submitCohortCsv' import './CreateSegmentFromCsv.scss' const PREVIEW_ROW_COUNT = 5 @@ -91,6 +93,11 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { [csvColumn], ) + const ignoredRowCount = extraction + ? extraction.emptyCount + + extraction.duplicateCount + + extraction.tooLongCount + : 0 const isBlocked = !!extraction && !extraction.identifiers.length const canSubmit = !!name && @@ -112,26 +119,28 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { return } try { - // Keep the created cohort across a failed sync so retrying only syncs. - let cohortId = createdCohortId - if (cohortId === null) { - const cohort = await createCohort({ - description: description || undefined, - environmentApiKey: environmentId, - metadata, - name, - projectId: Number(projectId), - }).unwrap() - cohortId = cohort.id - setCreatedCohortId(cohortId) - } - const result = await syncCohortCsv({ - cohortId, - environmentApiKey: environmentId, - file: new File([csvColumn], 'identifiers.csv', { type: 'text/csv' }), - has_header: false, - projectId: Number(projectId), - }).unwrap() + const result = await submitCohortCsv({ + createCohort: () => + createCohort({ + description: description || undefined, + environmentApiKey: environmentId, + metadata, + name, + projectId: Number(projectId), + }).unwrap(), + existingCohortId: createdCohortId, + onCohortCreated: setCreatedCohortId, + syncCsv: (cohortId) => + syncCohortCsv({ + cohortId, + environmentApiKey: environmentId, + file: new File([csvColumn], 'identifiers.csv', { + type: 'text/csv', + }), + has_header: false, + projectId: Number(projectId), + }).unwrap(), + }) toast( `Segment created with ${result.added} ${ result.added === 1 ? 'identity' : 'identities' @@ -309,7 +318,11 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { {extraction.identifiers.length === 1 ? 'identifier' : 'identifiers'}{' '} - detected. Duplicates and empty rows will be ignored. + detected. + {!!ignoredRowCount && + ` ${ignoredRowCount.toLocaleString()} ${ + ignoredRowCount === 1 ? 'row' : 'rows' + } ignored (empty, duplicate, or over ${MAX_IDENTIFIER_BYTES.toLocaleString()} bytes).`}
)} diff --git a/frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts b/frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts new file mode 100644 index 000000000000..66c847657b61 --- /dev/null +++ b/frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts @@ -0,0 +1,55 @@ +import { submitCohortCsv } from 'components/modals/CreateSegmentFromCsv/submitCohortCsv' + +describe('submitCohortCsv', () => { + test('first submit creates the cohort, records it, then syncs', async () => { + const createCohort = jest.fn().mockResolvedValue({ id: 7 }) + const syncCsv = jest.fn().mockResolvedValue({ added: 3 }) + const onCohortCreated = jest.fn() + + const result = await submitCohortCsv({ + createCohort, + existingCohortId: null, + onCohortCreated, + syncCsv, + }) + + expect(createCohort).toHaveBeenCalledTimes(1) + expect(onCohortCreated).toHaveBeenCalledWith(7) + expect(syncCsv).toHaveBeenCalledWith(7) + expect(result).toEqual({ added: 3 }) + }) + + test('retry with an existing cohort skips creation and only syncs', async () => { + const createCohort = jest.fn() + const syncCsv = jest.fn().mockResolvedValue({ added: 3 }) + const onCohortCreated = jest.fn() + + await submitCohortCsv({ + createCohort, + existingCohortId: 7, + onCohortCreated, + syncCsv, + }) + + expect(createCohort).not.toHaveBeenCalled() + expect(onCohortCreated).not.toHaveBeenCalled() + expect(syncCsv).toHaveBeenCalledWith(7) + }) + + test('a failed sync still records the created cohort for retry', async () => { + const createCohort = jest.fn().mockResolvedValue({ id: 7 }) + const syncCsv = jest.fn().mockRejectedValue(new Error('sync failed')) + const onCohortCreated = jest.fn() + + await expect( + submitCohortCsv({ + createCohort, + existingCohortId: null, + onCohortCreated, + syncCsv, + }), + ).rejects.toThrow('sync failed') + + expect(onCohortCreated).toHaveBeenCalledWith(7) + }) +}) diff --git a/frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts b/frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts new file mode 100644 index 000000000000..1c89d632a9f4 --- /dev/null +++ b/frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts @@ -0,0 +1,24 @@ +export type SubmitCohortCsvArgs = { + // The cohort created by an earlier failed attempt, so a retry only syncs. + existingCohortId: number | null + createCohort: () => Promise<{ id: number }> + syncCsv: (cohortId: number) => Promise + onCohortCreated: (cohortId: number) => void +} + +// Two-step save: create the cohort unless one survived a failed attempt, +// then sync the CSV. onCohortCreated fires before the sync so a failed sync +// still records the cohort id for retry. +export async function submitCohortCsv({ + createCohort, + existingCohortId, + onCohortCreated, + syncCsv, +}: SubmitCohortCsvArgs): Promise { + let cohortId = existingCohortId + if (cohortId === null) { + cohortId = (await createCohort()).id + onCohortCreated(cohortId) + } + return syncCsv(cohortId) +} diff --git a/frontend/web/components/segments/SegmentRow/SegmentRow.tsx b/frontend/web/components/segments/SegmentRow/SegmentRow.tsx index 0a3486aa646d..89ecb73f844d 100644 --- a/frontend/web/components/segments/SegmentRow/SegmentRow.tsx +++ b/frontend/web/components/segments/SegmentRow/SegmentRow.tsx @@ -10,7 +10,6 @@ import { SegmentMembershipTotalBadge } from 'components/segments/SegmentMembersh import Chip from 'components/base/Chip' import ConfirmCloneSegment from 'components/modals/ConfirmCloneSegment' import { useCloneSegmentMutation } from 'common/services/useSegment' -import { useGetEnvironmentsQuery } from 'common/services/useEnvironment' import { handleRemoveSegment } from 'components/modals/ConfirmRemoveSegment' import { ProjectPermission } from 'common/types/permissions.types' @@ -24,13 +23,6 @@ const SegmentRow: FC = ({ index, projectId, segment }) => { const history = useHistory() const { cohort, description, feature, id, name } = segment - const { data: environments } = useGetEnvironmentsQuery( - { projectId: Number(projectId) }, - { skip: !cohort }, - ) - const cohortEnvironment = environments?.results?.find( - (environment) => environment.id === cohort?.environment, - ) const isPendingDeletion = !!cohort?.deletion_requested_at const { permission: manageSegmentsPermission } = useHasPermission({ @@ -106,9 +98,9 @@ const SegmentRow: FC = ({ index, projectId, segment }) => { {cohort.source_type.toUpperCase()} )} - {!!cohort && !!cohortEnvironment && ( + {!!cohort && ( - {cohortEnvironment.name} + {cohort.environment_name} )} {isPendingDeletion && ( From f1386ba2dfd06b3dc8eddfea0a0262b7d76a10d7 Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 25 Aug 2026 11:34:59 +0200 Subject: [PATCH 3/5] fix: scope cohort retry to its form state and make cohort segments read-only --- .../web/components/modals/CreateSegment.tsx | 19 ++++++++--- .../CreateSegmentFromCsv.tsx | 17 +++++++--- .../__tests__/submitCohortCsv.test.ts | 33 +++++++++++++++---- .../CreateSegmentFromCsv/submitCohortCsv.ts | 31 +++++++++++------ .../modals/CreateSegmentRulesTabForm.tsx | 6 +++- .../segments/SegmentRow/SegmentRow.tsx | 3 +- 6 files changed, 82 insertions(+), 27 deletions(-) diff --git a/frontend/web/components/modals/CreateSegment.tsx b/frontend/web/components/modals/CreateSegment.tsx index dceff143c620..4d0f1df14f4f 100644 --- a/frontend/web/components/modals/CreateSegment.tsx +++ b/frontend/web/components/modals/CreateSegment.tsx @@ -143,6 +143,12 @@ const CreateSegment: FC = ({ projectId, }) const [segment, setSegment] = useState(_segment || defaultSegment) + // A cohort owns its segment's rules, and the API rejects updating one. + const isCohortManaged = !!_segment?.cohort + const isReadOnly = readOnly || isCohortManaged + const readOnlyMessage = isCohortManaged + ? 'This segment is managed by a cohort. Its rules follow the cohort membership and cannot be edited.' + : undefined const [description, setDescription] = useState(segment.description) const [name, setName] = useState(segment.name) const [rules, setRules] = useState(segment.rules) @@ -462,7 +468,7 @@ const CreateSegment: FC = ({ /> = ({ )} - {!readOnly && ( + {!isReadOnly && (
addRule(topLevelRuleType === 'ANY' ? 'ALL' : 'ANY') @@ -574,7 +580,8 @@ const CreateSegment: FC = ({ description={description} setDescription={setDescription} identity={identity} - readOnly={readOnly} + readOnly={isReadOnly} + readOnlyMessage={readOnlyMessage} showDescriptions={showDescriptions} setShowDescriptions={setShowDescriptions} allWarnings={allWarnings} @@ -653,7 +660,8 @@ const CreateSegment: FC = ({ description={description} setDescription={setDescription} identity={identity} - readOnly={readOnly} + readOnly={isReadOnly} + readOnlyMessage={readOnlyMessage} showDescriptions={showDescriptions} setShowDescriptions={setShowDescriptions} allWarnings={allWarnings} @@ -692,7 +700,8 @@ const CreateSegment: FC = ({ description={description} setDescription={setDescription} identity={identity} - readOnly={readOnly} + readOnly={isReadOnly} + readOnlyMessage={readOnlyMessage} showDescriptions={showDescriptions} setShowDescriptions={setShowDescriptions} allWarnings={allWarnings} diff --git a/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx b/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx index 61cd1e197d29..bda16c05bdbd 100644 --- a/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx +++ b/frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx @@ -29,7 +29,7 @@ import Icon from 'components/icons/Icon' import AddMetadataToEntity from 'components/metadata/AddMetadataToEntity' import TabItem from 'components/navigation/TabMenu/TabItem' import Tabs from 'components/navigation/TabMenu/Tabs' -import { submitCohortCsv } from './submitCohortCsv' +import { CreatedCohort, submitCohortCsv } from './submitCohortCsv' import './CreateSegmentFromCsv.scss' const PREVIEW_ROW_COUNT = 5 @@ -50,7 +50,7 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { const [selectedColumn, setSelectedColumn] = useState(null) const [tab, setTab] = useState(0) const [metadata, setMetadata] = useState([]) - const [createdCohortId, setCreatedCohortId] = useState(null) + const [createdCohort, setCreatedCohort] = useState(null) const [createCohort, { error: createError, isLoading: isCreating }] = useCreateCohortMutation() @@ -93,6 +93,14 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { [csvColumn], ) + // Identifies the cohort a retry may reuse; see submitCohortCsv. + const cohortFormKey = JSON.stringify({ + description, + environmentId, + metadata, + name, + }) + const ignoredRowCount = extraction ? extraction.emptyCount + extraction.duplicateCount + @@ -128,8 +136,9 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { name, projectId: Number(projectId), }).unwrap(), - existingCohortId: createdCohortId, - onCohortCreated: setCreatedCohortId, + createdCohort, + formKey: cohortFormKey, + onCohortCreated: setCreatedCohort, syncCsv: (cohortId) => syncCohortCsv({ cohortId, diff --git a/frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts b/frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts index 66c847657b61..c69812825bf9 100644 --- a/frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts +++ b/frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts @@ -8,25 +8,27 @@ describe('submitCohortCsv', () => { const result = await submitCohortCsv({ createCohort, - existingCohortId: null, + createdCohort: null, + formKey: 'a', onCohortCreated, syncCsv, }) expect(createCohort).toHaveBeenCalledTimes(1) - expect(onCohortCreated).toHaveBeenCalledWith(7) + expect(onCohortCreated).toHaveBeenCalledWith({ formKey: 'a', id: 7 }) expect(syncCsv).toHaveBeenCalledWith(7) expect(result).toEqual({ added: 3 }) }) - test('retry with an existing cohort skips creation and only syncs', async () => { + test('retry with an unchanged form reuses the cohort and only syncs', async () => { const createCohort = jest.fn() const syncCsv = jest.fn().mockResolvedValue({ added: 3 }) const onCohortCreated = jest.fn() await submitCohortCsv({ createCohort, - existingCohortId: 7, + createdCohort: { formKey: 'a', id: 7 }, + formKey: 'a', onCohortCreated, syncCsv, }) @@ -36,6 +38,24 @@ describe('submitCohortCsv', () => { expect(syncCsv).toHaveBeenCalledWith(7) }) + test('retry after editing the form creates a new cohort instead of reusing it', async () => { + const createCohort = jest.fn().mockResolvedValue({ id: 9 }) + const syncCsv = jest.fn().mockResolvedValue({ added: 1 }) + const onCohortCreated = jest.fn() + + await submitCohortCsv({ + createCohort, + createdCohort: { formKey: 'a', id: 7 }, + formKey: 'b', + onCohortCreated, + syncCsv, + }) + + expect(createCohort).toHaveBeenCalledTimes(1) + expect(onCohortCreated).toHaveBeenCalledWith({ formKey: 'b', id: 9 }) + expect(syncCsv).toHaveBeenCalledWith(9) + }) + test('a failed sync still records the created cohort for retry', async () => { const createCohort = jest.fn().mockResolvedValue({ id: 7 }) const syncCsv = jest.fn().mockRejectedValue(new Error('sync failed')) @@ -44,12 +64,13 @@ describe('submitCohortCsv', () => { await expect( submitCohortCsv({ createCohort, - existingCohortId: null, + createdCohort: null, + formKey: 'a', onCohortCreated, syncCsv, }), ).rejects.toThrow('sync failed') - expect(onCohortCreated).toHaveBeenCalledWith(7) + expect(onCohortCreated).toHaveBeenCalledWith({ formKey: 'a', id: 7 }) }) }) diff --git a/frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts b/frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts index 1c89d632a9f4..3fab248848f7 100644 --- a/frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts +++ b/frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts @@ -1,24 +1,35 @@ +// A created cohort belongs to the form state that created it. +export type CreatedCohort = { + id: number + formKey: string +} + export type SubmitCohortCsvArgs = { - // The cohort created by an earlier failed attempt, so a retry only syncs. - existingCohortId: number | null + // The cohort created by an earlier attempt, kept so a retry only syncs. + createdCohort: CreatedCohort | null + // Fingerprint of the inputs that feed cohort creation. + formKey: string createCohort: () => Promise<{ id: number }> syncCsv: (cohortId: number) => Promise - onCohortCreated: (cohortId: number) => void + onCohortCreated: (cohort: CreatedCohort) => void } -// Two-step save: create the cohort unless one survived a failed attempt, -// then sync the CSV. onCohortCreated fires before the sync so a failed sync -// still records the cohort id for retry. +// Two-step save: create the cohort, then sync the CSV. A cohort left behind by +// a failed sync is reused only while the inputs that created it are unchanged, +// so an edited retry never syncs against the previous form's cohort. +// onCohortCreated fires before the sync so a failed sync can still retry. export async function submitCohortCsv({ createCohort, - existingCohortId, + createdCohort, + formKey, onCohortCreated, syncCsv, }: SubmitCohortCsvArgs): Promise { - let cohortId = existingCohortId - if (cohortId === null) { + let cohortId = + createdCohort?.formKey === formKey ? createdCohort.id : undefined + if (cohortId === undefined) { cohortId = (await createCohort()).id - onCohortCreated(cohortId) + onCohortCreated({ formKey, id: cohortId }) } return syncCsv(cohortId) } diff --git a/frontend/web/components/modals/CreateSegmentRulesTabForm.tsx b/frontend/web/components/modals/CreateSegmentRulesTabForm.tsx index 0ea6c89708b1..4e576b9ab4cf 100644 --- a/frontend/web/components/modals/CreateSegmentRulesTabForm.tsx +++ b/frontend/web/components/modals/CreateSegmentRulesTabForm.tsx @@ -36,6 +36,8 @@ interface CreateSegmentRulesTabFormProps { setDescription: (description: string) => void identity?: boolean readOnly?: boolean + // Explains why editing is unavailable; defaults to the permission hint. + readOnlyMessage?: string showDescriptions: boolean setShowDescriptions: (show: boolean) => void allWarnings: string[] @@ -64,6 +66,7 @@ const CreateSegmentRulesTabForm: React.FC = ({ onCancel, onCreateChangeRequest, readOnly, + readOnlyMessage, rulesEl, save, segment, @@ -257,7 +260,8 @@ const CreateSegmentRulesTabForm: React.FC = ({ } place='left' > - {Constants.projectPermissions(ProjectPermission.ADMIN)} + {readOnlyMessage || + Constants.projectPermissions(ProjectPermission.ADMIN)}
) : ( diff --git a/frontend/web/components/segments/SegmentRow/SegmentRow.tsx b/frontend/web/components/segments/SegmentRow/SegmentRow.tsx index 89ecb73f844d..4f4d32b8f8c3 100644 --- a/frontend/web/components/segments/SegmentRow/SegmentRow.tsx +++ b/frontend/web/components/segments/SegmentRow/SegmentRow.tsx @@ -121,7 +121,8 @@ const SegmentRow: FC = ({ index, projectId, segment }) => { From 6bdb2c8adb4bbf89e77e43b0c53ab1e6f0ca80d2 Mon Sep 17 00:00:00 2001 From: wadii Date: Tue, 25 Aug 2026 12:30:42 +0200 Subject: [PATCH 4/5] fix: guard save and Add AND NOT button with isReadOnly for cohort-managed segments --- frontend/web/components/modals/CreateSegment.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frontend/web/components/modals/CreateSegment.tsx b/frontend/web/components/modals/CreateSegment.tsx index 4d0f1df14f4f..eb23e23f32c2 100644 --- a/frontend/web/components/modals/CreateSegment.tsx +++ b/frontend/web/components/modals/CreateSegment.tsx @@ -260,6 +260,7 @@ const CreateSegment: FC = ({ const save = async (e: FormEvent) => { try { Utils.preventDefault(e) + if (isReadOnly) return setValueChanged(false) setMetadataValueChanged(false) const segmentData: Omit = { @@ -510,7 +511,7 @@ const CreateSegment: FC = ({ )} - {topLevelRuleType !== 'ANY' && ( + {!isReadOnly && topLevelRuleType !== 'ANY' && (
addRule('NONE')} className='text-center'>