feat: create segment from CSV drawer - #8283
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds CSV parsing and identifier utilities with tests. Adds a drag-and-drop CSV upload component. Adds a segment creation form with header handling, identifier validation, row preview, metadata fields, and guarded submission. Derives segment sources from Flagsmith configuration. Routes active CSV and manual source selections through the segment page. Updates shared UI styling and modal title types. Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The PR adds a config-driven segment-source modal and CSV preview flow. Malformed source configuration can render an invalid card, and rapid file replacements can leave the preview showing an older selection; these are bounded UI correctness risks, so the change is mergeable with explicit owner follow-up. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bfbb06e to
497b491
Compare
github-actions
Bot
added
feature
|
@themis-blindfold review |
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
| for (let i = 0; i < text.length; i++) { | ||
| const char = text[i] |
There was a problem hiding this comment.
🧹 Nit · ⚡ Quick win
parseCsvText doesn't strip the UTF-8 BOM (U+FEFF)
Observed: a file starting with a BOM embeds the character in the first cell's value. Downstream .trim() in toParsedCsv and extractIdentifiers happens to strip it (ECMAScript's trim removes U+FEFF), so column names and identifiers are unaffected in practice. But the parser's own contract should handle it rather than relying on callers — the PR description lists BOM as a supported feature.
| for (let i = 0; i < text.length; i++) { | |
| const char = text[i] | |
| let pos = 0 | |
| if (text.charCodeAt(0) === 0xfeff) pos = 1 | |
| for (let i = pos; i < text.length; i++) { |
| </> | ||
| )} | ||
| <div className='text-right py-3'> | ||
| <Button data-test='create-segment' disabled={!canSubmit} type='submit'> |
There was a problem hiding this comment.
🧹 Nit · ⚡ Quick win
"Create Segment" button is enabled but does nothing
Once the form is filled out, the button is clickable but save is a no-op (e.preventDefault() only). Users testing behind the flag get zero feedback on click. Worth keeping it disabled (or labelled differently) until the follow-up PR wires submission.
⚖️ Themis review: 🧹 Ship it, nits insideTL;DR: Adds a flag-driven "Create Segment from CSV" drawer behind
🧹 Nits
📝 Walkthrough
🧪 How to verify
Product take: This is the front half of a CSV-based segment creation flow. The upload, preview, and validation UX is in good shape; the feature becomes useful once the follow-up PR wires submission to the cohorts API. Solid incremental progress on expanding segment sources beyond manual rules. 🧭 Assumptions & unverified claims
A form that accepts everything and submits nothing — the zen of incremental delivery. · reviewed at 497b491 |
github-actions
Bot
added
feature
Docker builds report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bdd094c7-b3fb-4fd2-aeea-71e98539edeb
📒 Files selected for processing (11)
frontend/common/utils/__tests__/csv.test.tsfrontend/common/utils/csv.tsfrontend/e2e/helpers/e2e-helpers.playwright.tsfrontend/web/components/CsvUpload/CsvUpload.scssfrontend/web/components/CsvUpload/CsvUpload.tsxfrontend/web/components/CsvUpload/index.tsfrontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsxfrontend/web/components/modals/CreateSegmentFromCsv/index.tsfrontend/web/components/modals/CreateSegmentSourcesModal/CreateSegmentSourcesModal.tsxfrontend/web/components/modals/CreateSegmentSourcesModal/index.tsfrontend/web/components/pages/SegmentsPage.tsx
| const { getInputProps, getRootProps } = useDropzone({ | ||
| accept: { | ||
| 'text/csv': ['.csv'], | ||
| }, | ||
| multiple: false, | ||
| onDrop, | ||
| onDropRejected: () => { | ||
| setError('Please select a CSV file') | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Add a maxSize limit to the dropzone.
The component accepts a CSV of any size. readAsText loads the whole file into a string, and the caller parses it synchronously on the main thread. A very large file freezes the tab with no feedback. Set maxSize and report the rejection through the existing onDropRejected path.
♻️ Proposed size guard
+const MAX_FILE_SIZE = 5 * 1024 * 1024
+
const { getInputProps, getRootProps } = useDropzone({
accept: {
'text/csv': ['.csv'],
},
+ maxSize: MAX_FILE_SIZE,
multiple: false,
onDrop,
- onDropRejected: () => {
- setError('Please select a CSV file')
- },
+ onDropRejected: (rejections) => {
+ const tooLarge = rejections.some((rejection) =>
+ rejection.errors.some((e) => e.code === 'file-too-large'),
+ )
+ setError(
+ tooLarge
+ ? 'The file is larger than 5 MB'
+ : 'Please select a CSV file',
+ )
+ },
})Please confirm that react-dropzone 14.2.3 reports the file-too-large error code in onDropRejected.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { getInputProps, getRootProps } = useDropzone({ | |
| accept: { | |
| 'text/csv': ['.csv'], | |
| }, | |
| multiple: false, | |
| onDrop, | |
| onDropRejected: () => { | |
| setError('Please select a CSV file') | |
| }, | |
| }) | |
| const MAX_FILE_SIZE = 5 * 1024 * 1024 | |
| const { getInputProps, getRootProps } = useDropzone({ | |
| accept: { | |
| 'text/csv': ['.csv'], | |
| }, | |
| maxSize: MAX_FILE_SIZE, | |
| multiple: false, | |
| onDrop, | |
| onDropRejected: (rejections) => { | |
| const tooLarge = rejections.some((rejection) => | |
| rejection.errors.some((e) => e.code === 'file-too-large'), | |
| ) | |
| setError( | |
| tooLarge | |
| ? 'The file is larger than 5 MB' | |
| : 'Please select a CSV file', | |
| ) | |
| }, | |
| }) |
| <div className='mb-3'> | ||
| <label>Environment*</label> | ||
| <EnvironmentSelect | ||
| projectId={Number(projectId)} | ||
| idField='id' | ||
| value={environmentId} | ||
| onChange={(value) => setEnvironmentId(`${value}`)} | ||
| /> | ||
| <div className='fs-small text-muted mt-1'> | ||
| The uploaded identities will be targeted in this environment only. | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- component map ---'
ast-grep outline frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx
printf '%s\n' '--- relevant component and select definitions ---'
rg -n --glob '*.ts' --glob '*.tsx' \
'EnvironmentSelect|Identifier column|data-test=.identifier-column-select.|inputId|htmlFor' \
frontend common 2>/dev/null | head -n 240
printf '%s\n' '--- target file ---'
cat -n frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx | sed -n '70,185p'Repository: Flagsmith/flagsmith
Length of output: 12820
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- EnvironmentSelect implementation ---'
cat -n frontend/web/components/EnvironmentSelect.tsx | sed -n '1,125p'
printf '%s\n' '--- Select component and package declarations ---'
rg -n --glob '*.ts' --glob '*.tsx' --glob 'package.json' --glob '*lock*' \
"from ['\"](common/|components/|project/|react-select)|react-select|<Select" \
frontend package.json yarn.lock package-lock.json pnpm-lock.yaml 2>/dev/null | head -n 260
printf '%s\n' '--- inputId usage and target identifiers ---'
rg -n --glob '*.ts' --glob '*.tsx' \
"inputId=['\"]segment-csv-environment|inputId=['\"]identifier-column-select|id=['\"]segment-csv-environment|id=['\"]identifier-column-select|htmlFor=['\"]segment-csv-environment|htmlFor=['\"]identifier-column-select" \
frontend 2>/dev/nullRepository: Flagsmith/flagsmith
Length of output: 31662
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
target = Path("frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx").read_text()
wrapper = Path("frontend/web/components/EnvironmentSelect.tsx").read_text()
checks = {
"environment label has no htmlFor": bool(re.search(r"<label>Environment\*</label>", target)),
"environment control has no inputId": bool(re.search(r"<EnvironmentSelect(?:(?!</EnvironmentSelect>).)*projectId=", target, re.S))
and not bool(re.search(r"<EnvironmentSelect(?:(?!</EnvironmentSelect>).)*inputId=", target, re.S)),
"identifier label has no htmlFor": bool(re.search(r"<label>Identifier column\*</label>", target)),
"identifier control has no inputId": bool(re.search(r"<Select(?:(?!/>).)*data-test='identifier-column-select'", target, re.S))
and not bool(re.search(r"<Select(?:(?!/>).)*inputId=", target, re.S)),
"EnvironmentSelect forwards rest to Select": bool(re.search(r"<Select\s*\{\.\.\.rest\}", wrapper)),
"EnvironmentSelect accepts inputId through react-select Props": bool(re.search(r"Partial<Omit<Props,\s*['\"]value['\"]>>", wrapper)),
}
for name, result in checks.items():
print(f"{name}: {'yes' if result else 'no'}")
if not all(checks.values()):
raise SystemExit("focused accessibility checks did not match the expected source shape")
PYRepository: Flagsmith/flagsmith
Length of output: 424
Associate both labels with their select controls.
Set matching htmlFor and inputId values for EnvironmentSelect and Select. EnvironmentSelect forwards inputId to its underlying Select.
| return (config as SegmentSourceFlagEntry[]) | ||
| .filter( | ||
| (entry) => entry?.visible !== false && SOURCE_DETAILS[entry?.name ?? ''], | ||
| ) | ||
| .map((entry) => ({ | ||
| ...SOURCE_DETAILS[entry.name ?? ''], | ||
| active: !!entry.active, | ||
| })) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check own properties of SOURCE_DETAILS before you accept a configured entry.
SOURCE_DETAILS[entry?.name ?? ''] also resolves inherited Object.prototype members. If the flag contains an entry named constructor, toString, or valueOf, the filter passes and the spread copies a function instead of source metadata. The resulting object has no key, name, or description, so the card renders empty, key={source.key} becomes undefined, and trackSourceEvent reports source: undefined. Restrict the lookup to own properties.
🐛 Proposed own-property check
return (config as SegmentSourceFlagEntry[])
- .filter(
- (entry) => entry?.visible !== false && SOURCE_DETAILS[entry?.name ?? ''],
- )
+ .filter(
+ (entry) =>
+ entry?.visible !== false &&
+ Object.prototype.hasOwnProperty.call(SOURCE_DETAILS, entry?.name ?? ''),
+ )
.map((entry) => ({
- ...SOURCE_DETAILS[entry.name ?? ''],
+ ...SOURCE_DETAILS[entry.name as string],
active: !!entry.active,
}))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return (config as SegmentSourceFlagEntry[]) | |
| .filter( | |
| (entry) => entry?.visible !== false && SOURCE_DETAILS[entry?.name ?? ''], | |
| ) | |
| .map((entry) => ({ | |
| ...SOURCE_DETAILS[entry.name ?? ''], | |
| active: !!entry.active, | |
| })) | |
| return (config as SegmentSourceFlagEntry[]) | |
| .filter( | |
| (entry) => | |
| entry?.visible !== false && | |
| Object.prototype.hasOwnProperty.call(SOURCE_DETAILS, entry?.name ?? ''), | |
| ) | |
| .map((entry) => ({ | |
| ...SOURCE_DETAILS[entry.name as string], | |
| active: !!entry.active, | |
| })) |
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19364 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #19364 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19364 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19364 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19347 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19347 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19347 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19347 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Skipped testsfirefox › tests/onboarding-tests.pw.ts › Onboarding › New user connects via the single-page onboarding flow @oss ✅ private-cloud · depot-ubuntu-latest-16 — run #19330 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19330 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19330 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19330 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
497b491 to
05b2fcf
Compare
github-actions
Bot
added
feature
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0dfea727-677f-4ea7-9da2-f7310e48cc5e
⛔ Files ignored due to path filters (1)
frontend/documentation/TokenReference.generated.stories.tsxis excluded by!**/*.generated.*
📒 Files selected for processing (15)
frontend/common/theme/tokens.jsonfrontend/common/theme/tokens.tsfrontend/common/utils/__tests__/csv.test.tsfrontend/common/utils/csv.tsfrontend/web/components/CsvUpload/CsvUpload.scssfrontend/web/components/CsvUpload/CsvUpload.tsxfrontend/web/components/EnvironmentSelect.tsxfrontend/web/components/base/forms/Checkbox.tsxfrontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.scssfrontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsxfrontend/web/components/modals/base/Modal.tsxfrontend/web/components/pages/SegmentsPage.tsxfrontend/web/styles/3rdParty/_react-select.scssfrontend/web/styles/_token-utilities.scssfrontend/web/styles/_tokens.scss
| const onDrop = useCallback( | ||
| (acceptedFiles: File[]) => { | ||
| setError('') | ||
| const file = acceptedFiles[0] | ||
| if (!file) { | ||
| return | ||
| } | ||
| const reader = new FileReader() | ||
| reader.addEventListener('load', () => { | ||
| onChange(file, `${reader.result}`) | ||
| }) | ||
| reader.addEventListener('error', () => { | ||
| setError('Error reading file') | ||
| }) | ||
| reader.readAsText(file) | ||
| }, | ||
| [onChange], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ignore stale file-read callbacks.
FileReader completes asynchronously. If the user selects a second file before the first read completes, the first listener can run last and replace the newer selection through onChange.
Track the current read and ignore load and error events from older readers.
Proposed fix
-import { FC, useCallback, useState } from 'react'
+import { FC, useCallback, useRef, useState } from 'react'
...
const CsvUpload: FC<CsvUploadType> = ({ onChange, rowCount, value }) => {
const [error, setError] = useState('')
+ const readVersion = useRef(0)
const onDrop = useCallback(
(acceptedFiles: File[]) => {
setError('')
const file = acceptedFiles[0]
if (!file) {
return
}
+ const version = ++readVersion.current
const reader = new FileReader()
reader.addEventListener('load', () => {
- onChange(file, `${reader.result}`)
+ if (version === readVersion.current && typeof reader.result === 'string') {
+ onChange(file, reader.result)
+ }
})
reader.addEventListener('error', () => {
- setError('Error reading file')
+ if (version === readVersion.current) {
+ setError('Error reading file')
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onDrop = useCallback( | |
| (acceptedFiles: File[]) => { | |
| setError('') | |
| const file = acceptedFiles[0] | |
| if (!file) { | |
| return | |
| } | |
| const reader = new FileReader() | |
| reader.addEventListener('load', () => { | |
| onChange(file, `${reader.result}`) | |
| }) | |
| reader.addEventListener('error', () => { | |
| setError('Error reading file') | |
| }) | |
| reader.readAsText(file) | |
| }, | |
| [onChange], | |
| const onDrop = useCallback( | |
| (acceptedFiles: File[]) => { | |
| setError('') | |
| const file = acceptedFiles[0] | |
| if (!file) { | |
| return | |
| } | |
| const version = ++readVersion.current | |
| const reader = new FileReader() | |
| reader.addEventListener('load', () => { | |
| if (version === readVersion.current && typeof reader.result === 'string') { | |
| onChange(file, reader.result) | |
| } | |
| }) | |
| reader.addEventListener('error', () => { | |
| if (version === readVersion.current) { | |
| setError('Error reading file') | |
| } | |
| }) | |
| reader.readAsText(file) | |
| }, | |
| [onChange], |
| readOnly?: boolean | ||
| idField?: 'id' | 'api_key' | ||
| ignore?: string[] | ||
| size?: 'default' | 'select-sm' | 'select-xsm' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the size union into a named type.
Define EnvironmentSelectSize and use it for the size property.
As per coding guidelines: frontend/**/*.{ts,tsx} must extract inline union types into named types.
Proposed change
+type EnvironmentSelectSize = 'default' | 'select-sm' | 'select-xsm'
+
type EnvironmentSelectType = Partial<Omit<Props, 'value'>> & {
...
- size?: 'default' | 'select-sm' | 'select-xsm'
+ size?: EnvironmentSelectSize
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size?: 'default' | 'select-sm' | 'select-xsm' | |
| type EnvironmentSelectSize = 'default' | 'select-sm' | 'select-xsm' | |
| type EnvironmentSelectType = Partial<Omit<Props, 'value'>> & { | |
| size?: EnvironmentSelectSize | |
| } |
Source: Coding guidelines
| type CreateSegmentFromCsvType = { | ||
| projectId: number | string | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the projectId union into a named type.
Line 26 defines an inline union type. Define a named type and use it for projectId.
Proposed fix
+type ProjectId = number | string
+
type CreateSegmentFromCsvType = {
- projectId: number | string
+ projectId: ProjectId
}As per coding guidelines, frontend/**/*.{ts,tsx} must extract inline union types into named types.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type CreateSegmentFromCsvType = { | |
| projectId: number | string | |
| } | |
| type ProjectId = number | string | |
| type CreateSegmentFromCsvType = { | |
| projectId: ProjectId | |
| } |
Source: Coding guidelines
05b2fcf to
033c67d
Compare
github-actions
Bot
added
feature
1 participant
docs/if required so people know about the feature.Changes
Adds the "New Segment — from a CSV list" drawer behind the segment sources modal, and makes the sources modal config-driven instead of hardcoded.
Flag-driven sources. The modal now reads its cards from the JSON value of the
create_segment_with_external_sourcesflag:[ { "active": true, "name": "csv", "visible": true }, { "active": false, "name": "amplitude", "visible": true }, { "active": false, "name": "mixpanel", "visible": true }, { "active": false, "name": "adobe_journey_manager", "visible": true } ]visible: falsehides a card,active: falserenders it as a Beta fake door,active: truemakes it real (currently only CSV has an implementation — an active source without one safely stays a fake door). Array order controls display order. Copy and icons stay in the frontend. Flag enabled with an empty or invalid value falls back to the manual drawer.CSV drawer.
How did you test this code?
Unit tests for the CSV parser/extractor (
common/utils/__tests__/csv.test.ts). Manually:create_segment_with_external_sourcesflag and enable it.[]: the button opens the manual drawer directly.Screens
Upload screen
Uploaded but unselected
Uploaded selected
Error states
