Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR changes Filters so opening the inline options picker for select/multiselect clears temporary selections, the popover renders multiselect fields as single-select, and each option click immediately commits a new Filter entry and closes the popover. Tests add a MultiselectTestFilters wrapper and assert two sequential option selections produce two committed filters and that the picker closes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
ref https://linear.app/ghost/issue/BER-3591/adding-a-second-label-filter-does-not-work The add-filter inline picker rendered multiselect fields (label, tier_id, offer_redemptions) as a sticky multi-select that stayed open after each click. Selecting a single value worked, but the open picker invited extra clicks, and each additional click left a stranded filter behind with stale single-value selections while a fresh filter was created with the latest cumulative selection. In multi-filter mode (allowMultiple={true}) the picker now treats multiselect fields as single-select: one click commits a new single-value filter and closes the picker. Multi-value editing of an existing filter continues through the filter row's own picker, which already supports live multi-select. Single-filter mode is unchanged.
ref https://linear.app/ghost/issue/BER-3591/adding-a-second-label-filter-does-not-work After the BER-3591 fix the inline "Add filter" picker is always single-pick + close, so several allowMultiple branches were unreachable in production or made redundant by that unified behaviour. - Dropped the "update existing filter" branch in addFilterWithOption. Audited all three consumers: members uses allowMultiple=true (skips the branch), comments only has select fields (closes after one pick), and stats's only multiselect field sets autoCloseOnSelect: true (same effect). The branch was unreachable. - Dropped the closePopover parameter — every commit now closes the popover. - Dropped the shouldClosePopover conditional and the redundant `allowMultiple &&` guard on the type-override at the picker call site. - Removed a dead setTempSelectedValues pre-populate in addFilter that read from a field already hidden by the field-list filter. - Tightened the surrounding comments. After cleanup, allowMultiple does exactly one thing: gates whether fields with an existing filter are hidden from the "Add filter" field list. No behaviour change.
8b318e3 to
8012348
Compare
ref https://linear.app/ghost/issue/BER-3591/adding-a-second-label-filter-does-not-work
Problem
In the React members list, opening the filter popover and picking "Label" (or any multiselect field) opened an inline multi-select picker that stayed open after each click and spawned a new filter on every commit. Picking a single value worked correctly, but the picker invited the user to click more — and each additional click left a stranded filter behind with stale single-value selections, while a fresh filter was created with the latest cumulative selection.
The same code path applied to every multiselect field in the members filter:
label,tier_id(Membership tier), andoffer_redemptions(Offer).Solution
The add-filter inline picker is now always single-pick: pick one value → commit a new single-value filter → close. Multi-value editing is the responsibility of the filter row's own picker (
LabelFilterRendererfor label, non-inlineSelectOptionsPopoverfor tier/offer), which already supports live multi-select with count updates.While in the area, several
allowMultiplebranches were either unreachable in production or redundant after the unified picker behaviour, so they were cleaned up:addFilterWithOptionwas unreachable. Audited all three consumers: members usesallowMultiple=true(skips that branch), comments only hasselectfields (closes after one pick), stats's only multiselect field setsautoCloseOnSelect: true(same effect). Removed.closePopoverparameter — every commit now closes the popover.shouldClosePopoverconditional and the deadaddFilterpre-populate that read from a hidden field.After the cleanup,
allowMultipledoes exactly one thing: gates whether fields with an existing filter are hidden from the "Add filter" field list (members shows them so users can stack same-field filters; comments and stats hide them so users get one filter per field). All other behaviour is uniform across both modes.Test plan
apps/shade/test/unit/components/ui/filters.test.tsxcovering the add-filter flow withallowMultiple={true}label— pick Blue Consultant, then add second filter Lime Orchestrator → 2 separate filters; chip dropdown adds Magenta to filter 2 in placetier_id— pick Silver, then add second filter Gold → 2 separate filters; chip dropdown adds Bronze to filter 2 in placeoffer_redemptions— pick Black Friday, then add second filter Free Trial → 2 separate filtersallowMultiple={false}consumers:pnpm --filter=@tryghost/shade test(213 passing)pnpm --filter=@tryghost/posts test(165 passing)pnpm --filter=@tryghost/shade lint