🎨 Used NQL $all operator for member label is-all-of filters#28596
🎨 Used NQL $all operator for member label is-all-of filters#28596rob-ghost wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx build @tryghost/portal |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/comments-ui |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/announcement-bar |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/activitypub |
✅ Succeeded | 1s | View ↗ |
nx build @tryghost/signup-form |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/admin-toolbar |
✅ Succeeded | 1s | View ↗ |
nx build @tryghost/sodo-search |
✅ Succeeded | <1s | View ↗ |
nx run @tryghost/admin-x-settings:test:acceptance |
✅ Succeeded | 10m 7s | View ↗ |
Additional runs (15) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-06-15 12:52:40 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pnpm-workspace.yaml`:
- Around line 135-140: Replace the non-portable local link overrides for
`@tryghost/nql` and `@tryghost/nql-lang` on lines 139-140 with catalog-pinned
versions. Add both dependencies to the catalog section of pnpm-workspace.yaml
with their published version numbers (or a documented temporary release tag),
then update the dependency declarations to reference them using the catalog:
syntax instead of link:../NQL paths. This ensures dependency resolution remains
reproducible across CI environments and contributor machines while respecting
the workspace's catalog-pinning workflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 727326e4-568c-4189-a02a-012a4f02cd21
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
apps/posts/src/views/filters/filter-codecs.test.tsapps/posts/src/views/filters/filter-codecs.tsapps/posts/src/views/filters/filter-query-core.test.tsapps/posts/src/views/filters/filter-query-core.tsapps/posts/src/views/filters/filter-types.tsapps/posts/src/views/filters/resolve-field.test.tsapps/posts/src/views/filters/resolve-field.tsapps/posts/src/views/members/member-filter-query.test.tsapps/posts/src/views/members/member-filter-query.tsghost/core/test/e2e-api/admin/members.test.jspnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- apps/posts/src/views/filters/filter-query-core.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## codex/ber-3664-member-label-is-all #28596 +/- ##
=====================================================================
Coverage ? 73.94%
=====================================================================
Files ? 1541
Lines ? 132388
Branches ? 15903
=====================================================================
Hits ? 97892
Misses ? 33532
Partials ? 964
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rob-ghost
force-pushed
the
codex/ber-3664-member-label-is-all
branch
from
76410ae to
4884ed7
Compare
Replaced the parenthesised-grouping workaround for "is all of" label filters with NQL's native all-of operator. The admin codecs and member filter query now serialize and parse the $all form directly, dropping the outer-grouping probe and the bespoke all-of extraction. Consumes the operator from a prerelease published under the `next` dist-tag (TryGhost/NQL#156) via the catalog plus an override that forces it onto transitive consumers like @tryghost/bookshelf-filter. Bump to the released versions once #156 merges to latest.
rob-ghost
force-pushed
the
feat/ber-3664-nql-all-of-operator
branch
from
d6f633d to
fe218a0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pnpm-workspace.yaml (1)
137-143:⚠️ Potential issue | 🟠 MajorReplace concrete versions in overrides with catalog references to maintain a single source of truth.
Lines 142-143 duplicate versions already pinned in the catalog (lines 63-64). With
catalogMode: strictenabled, these should reference the catalog directly using'catalog:'syntax, consistent with the pattern already used in line 145 for'@tryghost/logging'.Suggested fix
overrides: @@ - '`@tryghost/nql`': 0.13.1-pr156.0 - '`@tryghost/nql-lang`': 0.6.6-pr156.0 + '`@tryghost/nql`': 'catalog:' + '`@tryghost/nql-lang`': 'catalog:'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pnpm-workspace.yaml` around lines 137 - 143, Replace the concrete version numbers for '`@tryghost/nql`' and '`@tryghost/nql-lang`' in the overrides section with catalog references using the 'catalog:' syntax, matching the pattern already established for '`@tryghost/logging`'. This eliminates duplication since these versions are already defined in the catalog and ensures a single source of truth for version management when catalogMode is set to strict.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@pnpm-workspace.yaml`:
- Around line 137-143: Replace the concrete version numbers for '`@tryghost/nql`'
and '`@tryghost/nql-lang`' in the overrides section with catalog references using
the 'catalog:' syntax, matching the pattern already established for
'`@tryghost/logging`'. This eliminates duplication since these versions are
already defined in the catalog and ensures a single source of truth for version
management when catalogMode is set to strict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae60d459-9e13-4273-a8fc-5f02a729c7b5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/posts/src/views/filters/filter-codecs.test.tsapps/posts/src/views/filters/filter-codecs.tsapps/posts/src/views/filters/filter-types.tsapps/posts/src/views/filters/resolve-field.test.tsapps/posts/src/views/filters/resolve-field.tsapps/posts/src/views/members/member-filter-query.test.tsghost/core/test/e2e-api/admin/members.test.jspnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (1)
- ghost/core/test/e2e-api/admin/members.test.js
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/posts/src/views/filters/filter-types.ts
- apps/posts/src/views/filters/resolve-field.test.ts
- apps/posts/src/views/filters/resolve-field.ts
- apps/posts/src/views/members/member-filter-query.test.ts
- apps/posts/src/views/filters/filter-codecs.ts
- apps/posts/src/views/filters/filter-codecs.test.ts
2 participants
Problem
Member filtering supports an "is all of" mode for labels, where a member must carry every selected label. Until now the admin serialized this intent by hand-rolling a parenthesised group of separate clauses (one clause per label, ANDed together). That workaround was brittle: it duplicated NQL's grouping semantics in the admin, made outer-grouping detection fiddly (especially for quoted label values), and drifted from how the rest of the filter pipeline expresses queries.
Solution
This change replaces the parentheses-grouping workaround with first-class support for NQL's native "all of" operator, so an "is all of" label filter is now expressed as a single clause that lists every required label. The admin's filter codecs, query core, and member filter query all serialize and parse this form directly, and the field config drives whether a field is eligible for all-of handling rather than special-casing it in the query builder.
An end-to-end test drives the Members admin API over HTTP to prove the round trip: members are created with overlapping labels, and an "all of" filter returns only the member carrying every label, while the comma list still returns members carrying either.
This depends on the "all of" operator landing in NQL (TryGhost/NQL#156). While that is in review it is consumed as a prerelease published to npm under the
nextdist-tag (@tryghost/nql@0.13.1-pr156.0,@tryghost/nql-lang@0.6.6-pr156.0). The catalog points at those versions and a workspace override forces them onto transitive consumers too (the filter-to-SQL path lives in a transitive dependency). Once NQL#156 is released tolatest, the catalog and override get bumped to the stable versions and the override can be dropped.This PR is stacked on #27945.