feat: wire CSV segments to the cohorts API - #8295
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdded typed cohort APIs for creation, deletion, and CSV synchronisation. Added cohort request and response contracts. Added CSV column serialisation and upload size validation. Updated the CSV modal to create cohorts and synchronise identifier columns. Updated segment removal to use cohort deletion when applicable. Segment rows now show cohort details and deletion state. Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds CSV-backed cohort creation, synchronization, and deletion, but the current implementation can leave the UI stale, fail to remove a cohort during loading, or create a cohort whose upload is rejected when escaping expands the file beyond the API limit. Merge should wait for these bounded correctness issues to be addressed or explicitly accepted. 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 |
github-actions
Bot
added
the
front-end
Docker builds report
|
|
@themis-blindfold review |
✅ private-cloud · depot-ubuntu-latest-16 — run #19371 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19372 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19371 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19372 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19372 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19372 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
❌ oss · depot-ubuntu-latest-16 — run #19371 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/segment-test.pw.ts › Segment test 4 - Create ANY rule type segment and verify match changes when rule is updated @oss ✅ private-cloud · depot-ubuntu-latest-16 — run #19363 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19363 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19363 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19363 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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: 8b480a4e-0382-4485-8041-aa747fe926f9
📒 Files selected for processing (10)
frontend/common/services/useCohort.tsfrontend/common/types/requests.tsfrontend/common/types/responses.tsfrontend/common/utils/__tests__/csv.test.tsfrontend/common/utils/csv.tsfrontend/web/components/CsvUpload/CsvUpload.tsxfrontend/web/components/modals/ConfirmRemoveSegment.tsxfrontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsxfrontend/web/components/segments/SegmentRow/SegmentRow.tsxfrontend/web/components/segments/SegmentRow/components/SegmentAction.tsx
| syncCohortCsv: { | ||
| environmentApiKey: string | ||
| cohortId: number | ||
| file: File | ||
| identifier_column?: number | ||
| has_header?: boolean | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Invalidate the Segment list after CSV synchronisation.
CSV synchronisation changes cohort membership. createCohort and deleteCohort invalidate LIST${projectId} for Segment, but syncCohortCsv cannot do this because its request has no projectId. Segment membership counts can remain stale after a successful upload.
frontend/common/types/requests.ts#L185-L191: addprojectIdtoReq['syncCohortCsv'].frontend/common/services/useCohort.ts#L39-L39: invalidate{ id: \LIST${arg.projectId}`, type: 'Segment' }` with the Cohort tag.frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx#L113-L124: passprojectId: Number(projectId)tosyncCohortCsv.
📍 Affects 3 files
frontend/common/types/requests.ts#L185-L191(this comment)frontend/common/services/useCohort.ts#L39-L39frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsx#L113-L124
| segment.cohort && cohortEnvironmentApiKey | ||
| ? await deleteCohort(getStore(), { | ||
| cohortId: segment.cohort.id, | ||
| environmentApiKey: cohortEnvironmentApiKey, | ||
| projectId: Number(projectId), | ||
| }) | ||
| : await deleteSegment(getStore(), { id: segment.id, projectId }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not fall back to deleteSegment for a cohort-managed segment.
If segment.cohort exists but cohortEnvironmentApiKey is unavailable, lines 27-33 call deleteSegment. The segment endpoint rejects cohort-managed segments. This occurs if the user opens the removal flow before the environment query completes.
Reject the removal until the cohort environment API key is available. Keep deleteSegment only for segments without a cohort.
Proposed fix
+ const cohort = segment.cohort
+ if (cohort && !cohortEnvironmentApiKey) {
+ throw new Error('Cohort environment API key is unavailable')
+ }
const res =
- segment.cohort && cohortEnvironmentApiKey
+ cohort
? await deleteCohort(getStore(), {
- cohortId: segment.cohort.id,
+ cohortId: cohort.id,
environmentApiKey: cohortEnvironmentApiKey,
projectId: Number(projectId),
})
: await deleteSegment(getStore(), { id: segment.id, projectId })📝 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.
| segment.cohort && cohortEnvironmentApiKey | |
| ? await deleteCohort(getStore(), { | |
| cohortId: segment.cohort.id, | |
| environmentApiKey: cohortEnvironmentApiKey, | |
| projectId: Number(projectId), | |
| }) | |
| : await deleteSegment(getStore(), { id: segment.id, projectId }) | |
| const cohort = segment.cohort | |
| if (cohort && !cohortEnvironmentApiKey) { | |
| throw new Error('Cohort environment API key is unavailable') | |
| } | |
| const res = | |
| cohort | |
| ? await deleteCohort(getStore(), { | |
| cohortId: cohort.id, | |
| environmentApiKey: cohortEnvironmentApiKey, | |
| projectId: Number(projectId), | |
| }) | |
| : await deleteSegment(getStore(), { id: segment.id, projectId }) |
Visual Regression19 screenshots compared. See report for details. |
f5c68a3 to
5757a9d
Compare
github-actions
Bot
added
api
aa61a35 to
4083994
Compare
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
⚖️ Themis review: 🟠 Fix before mergeWires the CSV segment drawer to the cohort API with a clean two-step create-then-sync flow and idempotent retry logic. The segment list gains cohort chips and pending-deletion state handling. One deletion path was missed:
🟠 Majors
🧹 Nits
📝 Walkthrough
🧪 How to verify
Automate: add a unit/integration test that calls Product take: This delivers the end-to-end CSV segment flow — upload, parse, preview, create, sync — with thoughtful hardening (retry idempotency, client-side size cap, stack-overflow fix). Solid capability addition that closes the loop from PR #8283 and #8294. The gap in the detail-page deletion path should be fixed before shipping to avoid user confusion when deleting cohort segments. 🧭 Assumptions & unverified claims
A cohort's identifiers leave the browser but the detail page's delete button stays home. · reviewed at f5c68a3 |
71a387d to
3a5f36a
Compare
5757a9d to
166aae9
Compare
github-actions
Bot
added
feature
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Wires the CSV segment drawer (#8283) to the cohorts API (#8294).
Create flow
useCohortservice:createCohort,syncCohortCsv(multipart) anddeleteCohortmutations, invalidating the segment list.Segment list
CSVchip and a chip with the targeted environment's name, driven by thecohortsummary on the segment payload.Upload hardening
CsvUploadtakes amaxSizeBytesprop; the drawer passes 10MB to mirror the API cap, so oversized files are rejected client-side with a clear message.toParsedCsvno longer spreads every row intoMath.max, which overflowed the call stack on large files.How did you test this code?