feat(Segment membership inspection PoC): Surface identity counts in segments UI#7467
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Failed testsfirefox › tests/segment-membership-test.pw.ts › Segment membership badges render in list, tab, and env select @oss Details
Failed testsfirefox › tests/segment-membership-test.pw.ts › Segment membership badges render in list, tab, and env select @oss Details
Failed testsfirefox › tests/segment-membership-test.pw.ts › Segment membership badges render in list, tab, and env select @oss Details
Failed testsfirefox › tests/segment-membership-test.pw.ts › Segment membership badges render in list, tab, and env select @oss Details
Failed testsfirefox › tests/segment-membership-test.pw.ts › Segment membership badges render in list, tab, and env select @oss Details
Failed testsfirefox › tests/segment-membership-test.pw.ts › Segment membership badges render in list, tab, and env select @oss Details
Failed testsfirefox › tests/segment-membership-test.pw.ts › Segment membership badges render in list, tab, and env select @oss Details
Failed testsfirefox › tests/segment-membership-test.pw.ts › Segment membership badges render in list, tab, and env select @oss Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
|
I feel like we could neaten up the badges by putting the word identities and ~x days ago into a descriptive tooltip like we do for override badges. Other than that it looks great! |
|
@kyle-ssg The tooltips worked really badly with the tab and the environment select; that's how I came to dumping everything in the chip label. Besides that, I feel like last sync time is important info to surface before we switch to CDC / pure Snowflake lookups for counts. |
Zaimwa9
left a comment
There was a problem hiding this comment.
2 comments on my side:
- A subjective one related to the verbosity of the chip
- As environments are central to those components, it looks to me that we have some room for typing improvements, if you agree on it we could have another pass there sooner than later
89e88f9 to
6e1584b
Compare
|
is it ready for review @khvn26 ? |
Adds two reusable badges sourced from the Segment.memberships field: - SegmentMembershipTotalBadge: aggregates counts across environments and shows the most recent sync time as a relative interval. Rendered on each segment row in the project segments list (with the "identities" noun) and next to the Identities tab label on the segment edit page (compact, count + sync only). - SegmentMembershipEnvBadge: per-env count, rendered as the option label inside the Identities tab's environment select. Selecting an environment in the Identities tab displays the full last_synced_at timestamp underneath the select; before any selection the row stays in place as a placeholder. The shared Option component in project-components now honours selectProps.formatOptionLabel when callers provide one, falling back to the existing label/description layout. Drops the !important on the chip's align-self so badges can opt into vertical centering inline. beep boop
… tooltip Chip is now count-only; descriptive text (`N identities — last synced ~Xd ago`) lives in a tooltip on hover. The `compact` prop disappears since both call sites now render the same shape. beep boop
Env is always known at every call site, so drop the ProjectStore fallback that resolved it from `membership.environment`. The prop is now required. beep boop
…red option type Lifts the inline option shape out of EnvironmentSelect as EnvironmentSelectOption, then types the segment Identities-tab renderEnvOption argument directly instead of casting from `unknown`. beep boop
The previous regex required a literal '?', so the route mock never fired when RTK Query called /projects/<id>/environments/ with no params — the captured env list stayed empty and the test failed before its first assertion. beep boop
|
@talissoncosta Sorry, not quite ready yet — I'd like to fix the backend on staging first so the E2E test is more honest. |
|
No rush, take your time, lmk when I can have a look on it. |
The spec stubbed `memberships` into staging segment responses via `page.route`, so all the value of the test came from data the test itself injected. Dropping it removes the synthetic-data plumbing from CI and lets the real coverage land alongside the backend change that populates memberships for real. beep boop
|
@talissoncosta I've decided to remove the E2E test, thus unblocking this PR. This is now ready for a re-review. |
500ms is the right default for tooltips on hover-as-help-text controls, but the membership chip carries the canonical count + sync time and people will hover it deliberately. 100ms feels responsive while still filtering pass-through hovers. Default stays at 500ms for every other caller. beep boop
talissoncosta
left a comment
There was a problem hiding this comment.
Looks pretty good @khvn26 ! Good job! Just some minor comments, not blocking at all, but If you can clarify them before moving forward I would really appreciate it.
Drops the `ProjectStore.getEnvs()` flux read in favour of `useGetEnvironmentsQuery`. The legacy store was the last caller-side holdout in this component; everything else already runs through RTK Query. beep boop
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #7464 (backend merged).
Surfaces the new
Segment.membershipsfield on three places in the existing UI:last_synced_attimestamp underneath.Chip is count-only; a tooltip on hover surfaces
N identities — last synced ~Xd ago.Two small enabling changes outside the segments folder:
Optioninproject-components.jsnow honoursselectProps.formatOptionLabelwhen callers pass one (falls back to the existing label/description layout).!importanton the global.chipalign-selfso the badge can opt into vertical centering inline.Screenshots
Identity count chips in the Segments list:
Identity count chip on the Identities tab in the Segment page:
Per-environment chips:
Last sync date for selected environment:
How did you test this code?
Manually drove the segments list, segment edit page, and the Identities tab in a local dev server pointed at staging, with a fetch interceptor injecting synthetic
membershipsarrays into the segments responses. Verified the total badge on each row, the badge next to the Identities tab label, per-env chips inside the select options, the tooltip text on hover, and the full timestamp display on selection — including theLast synced: —placeholder before any selection.Added one Playwright spec (
e2e/tests/segment-membership-test.pw.ts) usingpage.routeto stubmembershipsend-to-end against a real seeded segment, asserting the total badge in the list, the badge on the tab, per-env badges in the select, and the full-timestamp line on selection.