feat(Segment membership inspection PoC): Surface identity counts in segments UI#7467
feat(Segment membership inspection PoC): Surface identity counts in segments UI#7467khvn26 wants to merge 5 commits into
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 |
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
| <UsersIcon className='chip-svg-icon' /> | ||
| <span> | ||
| {count} | ||
| {compact ? '' : ` ${noun}`} |
There was a problem hiding this comment.
I would either just get rid of mentioning "identities" as this badge has been already used or use compact everywhere (but it becomes dead code).
Regarding the last sync, this could be a tooltip on hover to not overload the UI
There was a problem hiding this comment.
I'm also thinking that we'll need to display the breakdown per environment for the identities. Could be through the tooltip but we can discuss the obstacles you had implementing it
There was a problem hiding this comment.
Addressed in f87b4eb — chip is now count-only, with N identities — last synced ~Xd ago in a hover tooltip.
There was a problem hiding this comment.
Holding off on per-env breakdown in the tooltip — that breakdown is already one click away inside the Identities tab (each env option carries its own chip + tooltip), and a tooltip on the list row would grow unbounded with env count.
| return map | ||
| }, [memberships]) | ||
|
|
||
| const renderEnvOption = (data: unknown) => { |
There was a problem hiding this comment.
We should be able to use data: EnvOption directly no ?
There was a problem hiding this comment.
Addressed in edfb511 — lifted the option shape into EnvironmentSelectOption (exported from EnvironmentSelect.tsx) and renderEnvOption now takes it directly instead of casting from unknown. environment stays nullable on the shared type to match the showAll placeholder, with a guard in the segment renderer.
|
|
||
| type EnvProps = { | ||
| membership: SegmentMembership | ||
| environment?: Environment |
There was a problem hiding this comment.
NIT typing: I feel like the environment should always be present in those components ? So it should be required here and cascaded in all these new components (e.g here)
There was a problem hiding this comment.
Addressed in 8e80b52 — environment is now required on SegmentMembershipEnvBadge, dropping the ProjectStore.getEnvs() fallback. Both call sites already had the env in hand.
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
a757fd1 to
52bf43f
Compare
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.