🐛 Fixed missing server-side label search in recipient & segment selects#26699
🐛 Fixed missing server-side label search in recipient & segment selects#26699kevinansfield merged 7 commits intomainfrom
Conversation
refs https://linear.app/tryghost/issue/ONC-1529 The paginated label loading refactor missed adding server-side search to these two dropdowns, requiring users to scroll through all pages before search would find labels not yet loaded client-side.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces token-based inputs with a new GhSegmentTokenInput and updates recipient/segment select components to use segment-focused props (nonLabelOptions, selectedSegments, hideLabelsComputed/hasSpecificOptions) instead of merged label+option arrays. GhSegmentTokenInput centralizes label loading, server-side search, mergedOptions, selectedOptions and a loadMoreLabelsTask via labelsManager. GhMembersSegmentSelect and GhMembersRecipientSelect remove label-management logic and expose non-label options/flags. Mirage labels route now supports server-side filtering and pagination. New integration tests exercise selection, pagination, and client/server search behaviors. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ghost/admin/tests/integration/components/gh-members-recipient-select-test.js (1)
69-69: Replace fixed sleeps with deterministic waits to reduce flakiness.The file contains 8 fixed 100ms delays at lines 69, 77, 84, 109, 113, 138, 142, and 146 that can intermittently fail in slower CI runs. Since
waitUntilis already imported and used in this test file (line 151), replace eachawait timeout(100);with explicit DOM or request condition checks. This pattern is already demonstrated in the existing code:await waitUntil(() => server.pretender.handledRequests.some(...)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/tests/integration/components/gh-members-recipient-select-test.js` at line 69, Replace the eight fixed sleeps (`await timeout(100);`) in the gh-members-recipient-select tests with deterministic waits: use the existing waitUntil pattern (e.g. await waitUntil(() => server.pretender.handledRequests.some(r => r.url.includes('...')))) or DOM-aware helpers (await find, await settled, or waitUntil(() => !!document.querySelector('...'))) to wait for the specific network request or element state the test relies on; update each occurrence to check the exact request URL/handler or element visibility/disabled state instead of a timeout so tests become deterministic (use the same request-checking pattern already used in this file with server.pretender.handledRequests and waitUntil).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/admin/app/components/gh-segment-token-input.js`:
- Around line 42-44: The getter useServerSideSearch currently returns
!this.labelsManager.hasLoadedAll but ignores hideLabels; update it to also
return false when this.hideLabels is true (e.g. return false if this.hideLabels
|| this.labelsManager.hasLoadedAll) and likewise update the other place around
lines 88-90 where the search wrapper/scroll can trigger loadMoreTask to bail out
early if this.hideLabels is true; ensure any logic that decides to
render/activate the search wrapper or call loadMoreTask checks this.hideLabels
before proceeding (referencing useServerSideSearch, this.hideLabels,
labelsManager.hasLoadedAll, and loadMoreTask).
In `@ghost/admin/tests/integration/components/gh-members-segment-select-test.js`:
- Around line 139-140: Tighten the server search assertion inside the waitUntil
call so it verifies the actual typed term is present: update the predicate that
checks server.pretender.handledRequests (used in waitUntil) to ensure
r.queryParams.filter both contains 'name:~' and also contains the specific term
'Label 50' (or the URL‑encoded equivalent) instead of only checking for 'name:~'
alone; modify the anonymous predicate used in the await waitUntil(...) to
require both substrings so the test fails if the typed term is missing.
- Line 33: Replace all fixed sleeps that call timeout(...) in the
gh-members-segment-select-test (the await timeout(...) usages) with
condition-based waits: after render() and after clickTrigger() use await
settled(); for places waiting on DOM updates or async state, use await
waitUntil(() => /* specific condition checking the expected DOM/state change,
e.g. element presence, text or class */). Update each occurrence that currently
uses timeout(...) so it awaits the appropriate settled() or waitUntil()
condition tied to the assertion that follows (use existing imports settled and
waitUntil and refer to the surrounding helpers like render and clickTrigger to
determine which to use).
---
Nitpick comments:
In
`@ghost/admin/tests/integration/components/gh-members-recipient-select-test.js`:
- Line 69: Replace the eight fixed sleeps (`await timeout(100);`) in the
gh-members-recipient-select tests with deterministic waits: use the existing
waitUntil pattern (e.g. await waitUntil(() =>
server.pretender.handledRequests.some(r => r.url.includes('...')))) or DOM-aware
helpers (await find, await settled, or waitUntil(() =>
!!document.querySelector('...'))) to wait for the specific network request or
element state the test relies on; update each occurrence to check the exact
request URL/handler or element visibility/disabled state instead of a timeout so
tests become deterministic (use the same request-checking pattern already used
in this file with server.pretender.handledRequests and waitUntil).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5278413f-9686-4ebd-8db7-fbf3997e061c
📥 Commits
Reviewing files that changed from the base of the PR and between be516c4 and 41757e74c7a50f401dc1f545bcfc4d61e5f7ac6b.
📒 Files selected for processing (9)
ghost/admin/app/components/gh-members-recipient-select.hbsghost/admin/app/components/gh-members-recipient-select.jsghost/admin/app/components/gh-members-segment-select.hbsghost/admin/app/components/gh-members-segment-select.jsghost/admin/app/components/gh-segment-token-input.hbsghost/admin/app/components/gh-segment-token-input.jsghost/admin/mirage/config/labels.jsghost/admin/tests/integration/components/gh-members-recipient-select-test.jsghost/admin/tests/integration/components/gh-members-segment-select-test.js
ghost/admin/tests/integration/components/gh-members-segment-select-test.js
Outdated
Show resolved
Hide resolved
ghost/admin/tests/integration/components/gh-members-segment-select-test.js
Outdated
Show resolved
Hide resolved
41757e7 to
5a59304
Compare
no issue Deduplicated label-related logic (labelsManager, search, infinite scroll, option merging) from `gh-members-segment-select` and `gh-members-recipient-select` into a shared `gh-segment-token-input component`. Added integration tests for both parent components and enhanced mirage labels config with filter support.
5a59304 to
c38b0df
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/admin/app/components/gh-members-recipient-select.js (1)
64-66: Optional: normalizeselectedSpecificSegmentsoutput.Trimming here avoids selection mismatch if incoming filter tokens contain spaces.
Proposed patch
get selectedSpecificSegments() { - return Array.from(this.specificFilters); + return Array.from(this.specificFilters) + .map(segment => segment.trim()) + .filter(Boolean); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/app/components/gh-members-recipient-select.js` around lines 64 - 66, selectedSpecificSegments currently returns Array.from(this.specificFilters) without normalizing items, so tokens with surrounding whitespace can cause selection mismatches; update the getter (selectedSpecificSegments) to map and trim each entry from Array.from(this.specificFilters) and remove any empty strings (e.g., filter out entries that become empty after trim) so consumers get consistent, whitespace-normalized segment identifiers from specificFilters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/admin/app/components/gh-members-segment-select.js`:
- Around line 24-35: The incoming segment parsing uses (this.args.segment ||
'').split(',') without trimming, so tokens with surrounding whitespace fail
includes() checks and matching; update the parsing in both the method that
defines the segments variable (used for the status:free / status:-free includes
checks and filtering logic) and the selectedSegments getter to normalize by
splitting then mapping each token through trim() and filtering falsy values,
i.e., replace raw split(...) usages with a normalizedTokens variable (or
similar) that does .split(',').map(t => t.trim()).filter(Boolean) and use that
for the includes(...) checks and return value of selectedSegments.
---
Nitpick comments:
In `@ghost/admin/app/components/gh-members-recipient-select.js`:
- Around line 64-66: selectedSpecificSegments currently returns
Array.from(this.specificFilters) without normalizing items, so tokens with
surrounding whitespace can cause selection mismatches; update the getter
(selectedSpecificSegments) to map and trim each entry from
Array.from(this.specificFilters) and remove any empty strings (e.g., filter out
entries that become empty after trim) so consumers get consistent,
whitespace-normalized segment identifiers from specificFilters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5c5ecb2-918c-410a-8f3e-7e7fbc966063
📥 Commits
Reviewing files that changed from the base of the PR and between 5a593045685bb198e8989516491c869f5aac507e and c38b0df.
📒 Files selected for processing (9)
ghost/admin/app/components/gh-members-recipient-select.hbsghost/admin/app/components/gh-members-recipient-select.jsghost/admin/app/components/gh-members-segment-select.hbsghost/admin/app/components/gh-members-segment-select.jsghost/admin/app/components/gh-segment-token-input.hbsghost/admin/app/components/gh-segment-token-input.jsghost/admin/mirage/config/labels.jsghost/admin/tests/integration/components/gh-members-recipient-select-test.jsghost/admin/tests/integration/components/gh-members-segment-select-test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- ghost/admin/mirage/config/labels.js
- ghost/admin/app/components/gh-segment-token-input.hbs
- ghost/admin/app/components/gh-members-recipient-select.hbs
- ghost/admin/app/components/gh-segment-token-input.js
- ghost/admin/tests/integration/components/gh-members-segment-select-test.js
…ults This update ensures that labels found via search, which are not part of the paginated set, are registered with the labelsManager. This enhancement allows selectedOptions to resolve correctly, improving the overall functionality of the label input component.
This reverts commit f4c6451.
The addLabel call was only in gh-segment-token-input but searchLabelsTask has multiple consumers. Moving registration into the service ensures all callers benefit. Added tests covering the scenario where a label found via search (outside the paginated set) resolves correctly after selection.
refs https://linear.app/tryghost/issue/ONC-1529
gh-members-recipient-selectandgh-members-segment-selectlabelsManager.searchLabelsTask) to both components, matching the pattern already used bygh-member-label-input