feat: cohort segment detail view with CSV re-synchronisation - #8387
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe API stores Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to CSV re-synchronisation can change feature-targeting membership, but concurrent uploads may produce an unintended combined result and the new editing flow may bypass the existing approval process for segment changes. These high-impact correctness and authorization risks should be resolved or explicitly accepted before merging. 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 |
Docker builds report
|
|
@themis-blindfold review |
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19923 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #19923 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19923 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19923 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19922 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19922 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19922 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19922 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19904 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19904 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19904 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #19904 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
⚖️ Themis review: 🟠 Fix before mergeThe CSV cohort detail view needs two fixes before merge: preserve read-only access in the replacement view, and report synchronisation progress from stable work totals rather than mutable membership state. Completed unit, lint, UI, image, and preview checks passed; E2E jobs were still running.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Automate: add component tests for read-only controls and add-only, remove-only, and mixed synchronisation progress. Local CSV utility tests could not be started because the checkout lacks frontend dependencies. Product take: This is a solid operational improvement for cohort-managed segments, but misleading progress and unavailable permissions make the primary management flow unreliable until fixed. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. A resync button should know which way the members are heading · reviewed at d4e34f7 |
c30addb to
a5f02fe
Compare
d4e34f7 to
c385cad
Compare
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeThe new cohort detail screen bypasses the page's read-only permission and reports a fixed, misleading progress percentage for removal-only synchronisations; both active major findings remain unresolved. The completed CI suite is green. Local typecheck could not be rerun because the TypeScript compiler is not installed in this checkout.
🟠 Majors
🧹 Nits
📝 Walkthrough
🧪 How to verify
Product take: A solid improvement to CSV cohort management, but view-only roles and removal-heavy synchronisations cannot be trusted until the two major issues are fixed. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. Two threads still need their cohort moment · reviewed at c385cad |
5a0375f to
2dff072
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8387 +/- ##
=======================================
Coverage 98.81% 98.81%
=======================================
Files 1620 1621 +1
Lines 66018 66102 +84
=======================================
+ Hits 65234 65318 +84
Misses 784 784 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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: 1148b737-93ba-47d2-bf29-d2b685e491c9
📒 Files selected for processing (25)
api/cohorts/migrations/0005_cohort_last_synced_at.pyapi/cohorts/models.pyapi/cohorts/serializers.pyapi/cohorts/services.pyapi/cohorts/views.pyapi/tests/unit/cohorts/test_views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mdfrontend/common/services/useCohort.tsfrontend/common/types/requests.tsfrontend/common/types/responses.tsfrontend/common/utils/csv.tsfrontend/web/components/CsvPreview/CsvPreview.scssfrontend/web/components/CsvPreview/CsvPreview.tsxfrontend/web/components/CsvPreview/index.tsfrontend/web/components/CsvUpload/CsvUpload.tsxfrontend/web/components/SegmentSelect.tsxfrontend/web/components/modals/CreateSegment.tsxfrontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.scssfrontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.tsxfrontend/web/components/segments/CohortSegmentDetail/CohortCsvSync.tsxfrontend/web/components/segments/CohortSegmentDetail/CohortSegmentDetail.scssfrontend/web/components/segments/CohortSegmentDetail/CohortSegmentDetail.tsxfrontend/web/components/segments/CohortSegmentDetail/index.tsfrontend/web/components/segments/SegmentRow/SegmentRow.tsxopenapi.yaml
💤 Files with no reviewable changes (2)
- frontend/web/components/modals/CreateSegmentFromCsv/CreateSegmentFromCsv.scss
- frontend/web/components/segments/SegmentRow/SegmentRow.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/web/components/segments/CohortSegmentDetail/CohortSegmentDetail.tsx (2)
74-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset synchronisation progress when the cohort changes.
When the same
CohortSegmentDetailinstance receives a new cohort,syncTotalremains from the previous cohort. A lowerpendingCountdoes not replace it because the effect appliesMath.max(total, pendingCount). Key the snapshot by cohort identity and initialise it from the new cohort’s pending work.
86-89: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReset the form baseline when the segment identity changes.
When
SegmentPagechanges the route, the existing component can receive a different segment. If both segments have the samenameanddescription, this effect does not reset the local form values. Includesegment.idin the dependency list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7c42fc81-f8c7-426f-ad51-71fa8d82118e
📒 Files selected for processing (3)
frontend/web/components/modals/CreateSegment.tsxfrontend/web/components/segments/CohortSegmentDetail/CohortSegmentDetail.scssfrontend/web/components/segments/CohortSegmentDetail/CohortSegmentDetail.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Detail view for CSV cohort-managed segments, replacing the read-only rules editor:
PATCHendpoint.CsvUploadand the new sharedCsvPreview, with a review summary before synchronising.SegmentSelectshows the CSV chip and hides segments pending deletion.Stacked on #8386.
How did you test this code?
Manually: created a CSV segment, re-uploaded a smaller file and verified the add/remove counts, progress and status against a running task processor; renamed the segment and checked list/breadcrumb refresh. Unit tests, eslint and typecheck (no new errors) pass.