Removed Ember members implementation#27599
Open
jonatansberg wants to merge 9 commits intomainfrom
Open
Conversation
Contributor
|
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:
WalkthroughThis PR removes a large portion of members-related admin UI and logic: many Ember/Glimmer components, templates, modals, the filter-builder and its CSS, import/export CSV tooling and validator service, bulk-member modals, member list templates/controllers/routes, Mirage CSV upload mocks, numerous filter/export tests, and related devDependencies. It adds a StateBridge action/event for member commenting changes, maps the Ember ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #27599 +/- ##
==========================================
+ Coverage 73.17% 73.77% +0.60%
==========================================
Files 1561 1511 -50
Lines 127051 125649 -1402
Branches 15383 15125 -258
==========================================
- Hits 92970 92701 -269
+ Misses 33105 32007 -1098
+ Partials 976 941 -35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 451a8e9127
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| window.adminXQueryClient.invalidateQueries({queryKey: ['CommentsResponseType']}); | ||
| window.adminXQueryClient.invalidateQueries({queryKey: ['MembersResponseType']}); | ||
| } | ||
| this.invalidateMembersCache(); |
There was a problem hiding this comment.
Calling invalidateMembersCache() here only emits an Ember data change for model member, which the React bridge maps to MembersResponseType invalidation only (apps/admin/src/ember-bridge/ember-bridge.tsx). Before this change, enabling commenting explicitly invalidated both MembersResponseType and CommentsResponseType, so after toggling commenting on from member detail, cached Comments views can remain stale until a hard refresh.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ghost/admin/app/controllers/member.js (1)
187-193:⚠️ Potential issue | 🟠 MajorInvalidate the React members cache here too.
confirmDisableCommenting()still only refetches the Ember member, so React can remain stale after this mutation. The new helper is already the shared path for the other member mutations, so this callback should use it as well.Suggested fix
afterDisable: () => { this.fetchMemberTask.perform(this.member.id); + this.invalidateMembersCache(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/app/controllers/member.js` around lines 187 - 193, confirmDisableCommenting currently only refetches the Ember member via this.fetchMemberTask.perform(this.member.id) so the React members list can remain stale; update the afterDisable callback in confirmDisableCommenting to also invoke the shared member cache invalidation helper used by the other member mutations (the same helper other mutations call) so the React cache is invalidated as well—keep the existing fetchMemberTask.perform call if needed for Ember state, but ensure the shared invalidate-members-cache helper is called from afterDisable.ghost/admin/mirage/routes-test.js (1)
88-110:⚠️ Potential issue | 🔴 CriticalThis unconditional block will break 13+ legitimate
limit=allcallers across the codebase.The guard at lines 88-90 throws before the endpoint-specific allowlist, blocking all
limit=allrequests unconditionally. However, many production code paths still actively uselimit=all:
app/routes/posts.js,app/controllers/lexical-editor.js,app/utils/publish-options.jsapp/services/members-utils.js,app/services/members-count-cache.js,app/services/limit.jsapp/components/gh-members-recipient-select.js,app/components/posts/debug.js,app/components/koenig-lexical-editor.js,app/components/gh-psm-authors-input.js,app/components/gh-members-segment-select.js,app/components/gh-post-settings-menu/visibility-segment-select.jsThis change will break existing features (newsletters, members, tiers, snippets, posts, users). Either remove the unconditional block or add specific allowlist rules for these endpoints before landing.
e2e/helpers/pages/admin/members/members-list-page.ts (1)
144-149:⚠️ Potential issue | 🟠 MajorAdd null check for download path before reading file.
download.path()can returnnull, and theas stringcast masks this from TypeScript. Explicitly handle the null case before passing toreadFileSync().Proposed fix
const download = await this.exportMembersData(); const suggestedFilename = download.suggestedFilename(); const downloadPath = await download.path(); - const downloadContent = readFileSync(downloadPath as string, 'utf-8'); + if (!downloadPath) { + throw new Error('Download path is unavailable in this test environment'); + } + const downloadContent = readFileSync(downloadPath, 'utf-8');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/members/members-list-page.ts` around lines 144 - 149, In exportMembers(), handle the case where download.path() can return null before calling readFileSync: call const downloadPath = await download.path(); check if downloadPath is null (or undefined) and throw or return a clear error/empty result, then only call readFileSync(downloadPath, 'utf-8'); update exportMembers to reference exportMembersData(), suggestedFilename() and readFileSync so the null path branch prevents passing a coerced string to readFileSync.
🤖 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/styles/layouts/members.css`:
- Around line 91-93: The rule is using the deprecated CSS property
grid-column-gap in the selector .gh-member-settings .gh-main-section.columns-3;
replace that property with the modern column-gap property (keep the same value
48px) so the stylesheet uses current CSS syntax and satisfies linters expecting
column-gap instead of grid-column-gap.
---
Outside diff comments:
In `@e2e/helpers/pages/admin/members/members-list-page.ts`:
- Around line 144-149: In exportMembers(), handle the case where download.path()
can return null before calling readFileSync: call const downloadPath = await
download.path(); check if downloadPath is null (or undefined) and throw or
return a clear error/empty result, then only call readFileSync(downloadPath,
'utf-8'); update exportMembers to reference exportMembersData(),
suggestedFilename() and readFileSync so the null path branch prevents passing a
coerced string to readFileSync.
In `@ghost/admin/app/controllers/member.js`:
- Around line 187-193: confirmDisableCommenting currently only refetches the
Ember member via this.fetchMemberTask.perform(this.member.id) so the React
members list can remain stale; update the afterDisable callback in
confirmDisableCommenting to also invoke the shared member cache invalidation
helper used by the other member mutations (the same helper other mutations call)
so the React cache is invalidated as well—keep the existing
fetchMemberTask.perform call if needed for Ember state, but ensure the shared
invalidate-members-cache helper is called from afterDisable.
🪄 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: 65af68c3-f6de-4e3c-bdc9-2930a5c217e5
⛔ Files ignored due to path filters (10)
ghost/admin/public/assets/icons/email-member.svgis excluded by!**/*.svgghost/admin/public/assets/icons/import-in-progress.svgis excluded by!**/*.svgghost/admin/public/assets/icons/members-all.svgis excluded by!**/*.svgghost/admin/public/assets/icons/members-outline.svgis excluded by!**/*.svgghost/admin/public/assets/icons/members-paid.svgis excluded by!**/*.svgghost/admin/public/assets/icons/members-post.svgis excluded by!**/*.svgghost/admin/public/assets/icons/members-segment.svgis excluded by!**/*.svgghost/admin/public/assets/img/marketing/members-1.jpgis excluded by!**/*.jpgghost/admin/public/assets/img/marketing/members-2.jpgis excluded by!**/*.jpgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (99)
apps/admin/src/ember-bridge/ember-bridge.test.tsxapps/admin/src/ember-bridge/ember-bridge.tsxe2e/helpers/pages/admin/members/members-list-page.tse2e/helpers/pages/admin/members/members-page.tsghost/admin/app/components/gh-member-single-label-input.hbsghost/admin/app/components/gh-member-single-label-input.jsghost/admin/app/components/gh-members-import-mapping-input.hbsghost/admin/app/components/gh-members-import-mapping-input.jsghost/admin/app/components/gh-members-import-table.hbsghost/admin/app/components/gh-members-import-table.jsghost/admin/app/components/gh-members-no-members.hbsghost/admin/app/components/gh-members-no-members.jsghost/admin/app/components/members/filter-value.hbsghost/admin/app/components/members/filter-value.jsghost/admin/app/components/members/filter.hbsghost/admin/app/components/members/filter.jsghost/admin/app/components/members/filters/audience-feedback.jsghost/admin/app/components/members/filters/columns/date-column.jsghost/admin/app/components/members/filters/created-at.jsghost/admin/app/components/members/filters/email-clicked.jsghost/admin/app/components/members/filters/email-count.jsghost/admin/app/components/members/filters/email-open-rate.jsghost/admin/app/components/members/filters/email-opened-count.jsghost/admin/app/components/members/filters/email-opened.jsghost/admin/app/components/members/filters/email-sent.jsghost/admin/app/components/members/filters/email.jsghost/admin/app/components/members/filters/index.jsghost/admin/app/components/members/filters/label.jsghost/admin/app/components/members/filters/last-seen.jsghost/admin/app/components/members/filters/name.jsghost/admin/app/components/members/filters/next-billing-date.jsghost/admin/app/components/members/filters/offers.jsghost/admin/app/components/members/filters/plan-interval.jsghost/admin/app/components/members/filters/relation-options/contains.jsghost/admin/app/components/members/filters/relation-options/date.jsghost/admin/app/components/members/filters/relation-options/index.jsghost/admin/app/components/members/filters/relation-options/match.jsghost/admin/app/components/members/filters/relation-options/number.jsghost/admin/app/components/members/filters/signup-attribution.jsghost/admin/app/components/members/filters/status.jsghost/admin/app/components/members/filters/subscribed.jsghost/admin/app/components/members/filters/subscription-attribution.jsghost/admin/app/components/members/filters/subscription-start-date.jsghost/admin/app/components/members/filters/subscription-status.jsghost/admin/app/components/members/filters/tier.jsghost/admin/app/components/members/list-item-column.hbsghost/admin/app/components/members/list-item-column.jsghost/admin/app/components/members/list-item-loading.hbsghost/admin/app/components/members/list-item.hbsghost/admin/app/components/members/list-item.jsghost/admin/app/components/members/modals/bulk-add-label.hbsghost/admin/app/components/members/modals/bulk-add-label.jsghost/admin/app/components/members/modals/bulk-delete.hbsghost/admin/app/components/members/modals/bulk-delete.jsghost/admin/app/components/members/modals/bulk-remove-label.hbsghost/admin/app/components/members/modals/bulk-remove-label.jsghost/admin/app/components/members/modals/bulk-unsubscribe.hbsghost/admin/app/components/members/modals/bulk-unsubscribe.jsghost/admin/app/components/modal-import-members.hbsghost/admin/app/components/modal-import-members.jsghost/admin/app/components/modal-import-members/csv-file-mapping.hbsghost/admin/app/components/modal-import-members/csv-file-mapping.jsghost/admin/app/components/modal-import-members/csv-file-select.hbsghost/admin/app/components/modal-import-members/csv-file-select.jsghost/admin/app/components/modal-unsubscribe-members.hbsghost/admin/app/components/modal-unsubscribe-members.jsghost/admin/app/components/offers/segment-select.hbsghost/admin/app/components/offers/segment-select.jsghost/admin/app/components/tiers/segment-select.hbsghost/admin/app/components/tiers/segment-select.jsghost/admin/app/controllers/member.jsghost/admin/app/controllers/members.jsghost/admin/app/controllers/members/import.jsghost/admin/app/errors/member-import-error.jsghost/admin/app/helpers/reset-query-params.jsghost/admin/app/routes/member.jsghost/admin/app/routes/members.jsghost/admin/app/routes/members/import.jsghost/admin/app/services/member-import-validator.jsghost/admin/app/services/state-bridge.jsghost/admin/app/styles/app-dark.cssghost/admin/app/styles/app.cssghost/admin/app/styles/components/filter-builder.cssghost/admin/app/styles/layouts/dashboard.cssghost/admin/app/styles/layouts/members.cssghost/admin/app/templates/members.hbsghost/admin/app/templates/members/import.hbsghost/admin/mirage/config/members.jsghost/admin/mirage/routes-test.jsghost/admin/package.jsonghost/admin/tests/acceptance/members-test.jsghost/admin/tests/acceptance/members/filter-test.jsghost/admin/tests/acceptance/members/import-test.jsghost/admin/tests/integration/components/gh-members-import-table-test.jsghost/admin/tests/integration/components/modal-import-members-test.jsghost/admin/tests/integration/services/member-import-validator-test.jsghost/admin/tests/unit/components/members/filters/offers-test.jsghost/admin/tests/unit/controllers/member-test.jsghost/admin/tests/unit/services/state-bridge-test.js
💤 Files with no reviewable changes (91)
- ghost/admin/app/routes/member.js
- apps/admin/src/ember-bridge/ember-bridge.tsx
- apps/admin/src/ember-bridge/ember-bridge.test.tsx
- ghost/admin/app/components/members/filters/relation-options/date.js
- ghost/admin/app/templates/members/import.hbs
- ghost/admin/app/components/members/filters/relation-options/match.js
- ghost/admin/app/components/members/filters/relation-options/contains.js
- ghost/admin/app/components/members/filters/tier.js
- ghost/admin/app/components/members/filters/created-at.js
- ghost/admin/app/components/members/list-item-column.hbs
- ghost/admin/app/components/gh-members-no-members.hbs
- ghost/admin/app/styles/app.css
- ghost/admin/app/routes/members/import.js
- ghost/admin/app/components/members/filters/email-clicked.js
- ghost/admin/app/components/members/filters/subscription-attribution.js
- ghost/admin/app/components/members/filters/relation-options/number.js
- ghost/admin/app/components/members/filters/label.js
- ghost/admin/app/components/members/list-item.hbs
- ghost/admin/app/components/members/filters/subscription-start-date.js
- ghost/admin/app/components/members/modals/bulk-remove-label.hbs
- ghost/admin/app/components/members/modals/bulk-delete.hbs
- ghost/admin/app/components/members/list-item-loading.hbs
- ghost/admin/app/components/members/filter-value.js
- ghost/admin/app/components/members/filters/offers.js
- ghost/admin/app/helpers/reset-query-params.js
- ghost/admin/app/components/members/filters/last-seen.js
- ghost/admin/app/components/members/filters/signup-attribution.js
- ghost/admin/app/components/members/filters/email-opened.js
- ghost/admin/app/components/members/filter.hbs
- ghost/admin/mirage/config/members.js
- ghost/admin/app/components/gh-members-no-members.js
- ghost/admin/app/components/gh-members-import-mapping-input.hbs
- ghost/admin/app/components/members/filters/email-opened-count.js
- ghost/admin/app/components/members/filters/email-count.js
- ghost/admin/app/components/members/filters/subscription-status.js
- ghost/admin/app/components/modal-unsubscribe-members.hbs
- ghost/admin/tests/unit/components/members/filters/offers-test.js
- ghost/admin/app/components/members/modals/bulk-unsubscribe.hbs
- ghost/admin/app/components/modal-import-members/csv-file-select.js
- ghost/admin/app/components/members/filters/status.js
- ghost/admin/app/components/gh-members-import-mapping-input.js
- ghost/admin/app/styles/components/filter-builder.css
- ghost/admin/tests/integration/components/gh-members-import-table-test.js
- ghost/admin/app/components/gh-member-single-label-input.hbs
- ghost/admin/app/components/members/filters/relation-options/index.js
- ghost/admin/tests/integration/components/modal-import-members-test.js
- ghost/admin/app/components/modal-unsubscribe-members.js
- ghost/admin/app/styles/layouts/dashboard.css
- ghost/admin/tests/integration/services/member-import-validator-test.js
- ghost/admin/app/components/members/filters/plan-interval.js
- ghost/admin/app/controllers/members.js
- ghost/admin/app/components/members/filters/next-billing-date.js
- ghost/admin/app/components/modal-import-members/csv-file-mapping.hbs
- ghost/admin/app/components/members/list-item.js
- ghost/admin/app/components/modal-import-members/csv-file-select.hbs
- ghost/admin/app/components/offers/segment-select.js
- ghost/admin/app/components/members/filter-value.hbs
- ghost/admin/tests/acceptance/members-test.js
- ghost/admin/app/routes/members.js
- ghost/admin/app/components/members/filters/email-open-rate.js
- ghost/admin/app/components/members/modals/bulk-add-label.hbs
- ghost/admin/app/components/offers/segment-select.hbs
- ghost/admin/app/components/members/filters/audience-feedback.js
- ghost/admin/app/components/members/modals/bulk-remove-label.js
- ghost/admin/app/components/modal-import-members.hbs
- ghost/admin/app/services/member-import-validator.js
- ghost/admin/tests/acceptance/members/import-test.js
- ghost/admin/app/components/members/filter.js
- ghost/admin/app/styles/app-dark.css
- ghost/admin/app/components/modal-import-members.js
- ghost/admin/app/components/members/filters/email-sent.js
- ghost/admin/tests/acceptance/members/filter-test.js
- ghost/admin/app/components/members/modals/bulk-delete.js
- ghost/admin/app/components/members/modals/bulk-unsubscribe.js
- ghost/admin/app/components/members/filters/email.js
- ghost/admin/app/components/members/list-item-column.js
- ghost/admin/app/components/members/modals/bulk-add-label.js
- ghost/admin/app/templates/members.hbs
- ghost/admin/app/errors/member-import-error.js
- ghost/admin/app/components/tiers/segment-select.js
- ghost/admin/app/components/gh-member-single-label-input.js
- ghost/admin/app/controllers/members/import.js
- ghost/admin/app/components/gh-members-import-table.js
- ghost/admin/app/components/members/filters/subscribed.js
- ghost/admin/app/components/tiers/segment-select.hbs
- ghost/admin/app/components/members/filters/index.js
- ghost/admin/app/components/members/filters/name.js
- ghost/admin/package.json
- ghost/admin/app/components/members/filters/columns/date-column.js
- ghost/admin/app/components/gh-members-import-table.hbs
- ghost/admin/app/components/modal-import-members/csv-file-mapping.js
Comment on lines
+91
to
93
| .gh-member-settings .gh-main-section.columns-3 { | ||
| grid-column-gap: 48px; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there are no deprecated `grid-column-gap` declarations left in CSS files.
# Expected result after fix: no matches.
rg -nP --type=css '\bgrid-column-gap\s*:'Repository: TryGhost/Ghost
Length of output: 718
Replace deprecated grid gap property.
Line 92 uses deprecated grid-column-gap; use column-gap instead to satisfy current CSS/lint expectations.
Suggested fix
.gh-member-settings .gh-main-section.columns-3 {
- grid-column-gap: 48px;
+ column-gap: 48px;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .gh-member-settings .gh-main-section.columns-3 { | |
| grid-column-gap: 48px; | |
| } | |
| .gh-member-settings .gh-main-section.columns-3 { | |
| column-gap: 48px; | |
| } |
🧰 Tools
🪛 Stylelint (17.9.0)
[error] 92-92: Expected "grid-column-gap" to be "column-gap" (property-no-deprecated)
(property-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/admin/app/styles/layouts/members.css` around lines 91 - 93, The rule is
using the deprecated CSS property grid-column-gap in the selector
.gh-member-settings .gh-main-section.columns-3; replace that property with the
modern column-gap property (keep the same value 48px) so the stylesheet uses
current CSS syntax and satisfies linters expecting column-gap instead of
grid-column-gap.
ref https://linear.app/ghost/issue/BER-3580/remove-ember-members-implementation React owns the members list and import routes now, so the old Ember route, controller, component, skipped test, and admin-only dependency surface can be deleted.
ref https://linear.app/ghost/issue/BER-3580/remove-ember-members-implementation Removing the Ember list surfaced orphaned helpers, Mirage upload mocks, legacy CSS, and public assets that are no longer referenced by the retained member detail or activity screens.
ref https://linear.app/ghost/issue/BER-3580/remove-ember-members-implementation The retained Ember member detail screen can no longer depend on the removed members controller, so it now invalidates the React members cache directly after member mutations.
ref https://linear.app/ghost/issue/BER-3580/remove-ember-members-implementation The retained Ember member detail screen should use the existing Ember bridge for React cache invalidation instead of reaching into the AdminX query client directly.
ref https://linear.app/ghost/issue/BER-3580/remove-ember-members-implementation Comments do not have an Ember model counterpart, so the retained member detail screen only emits member bridge updates after member mutations.
ref https://linear.app/ghost/issue/BER-3580/remove-ember-members-implementation The controller test now stubs the injected bridge dependency at the controller boundary instead of spying on the real service instance, which is not stable in the isolated Ember test owner.
jonatansberg
force-pushed
the
jonatan-ber-3580-remove-ember-members-implementation
branch
from
451a8e9 to
fb3f204
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/TryGhost/Ghost/blob/fb3f20433d644d58a7cf0f37b7d1fe727c64e71a/ghost/admin/public/assets/img/marketing/members-1.jpg#L1
Removing this image breaks the React members empty-state help cards, which still hardcode url(${assetRoot}img/marketing/members-1.jpg) and members-2.jpg in apps/posts/src/views/members/components/members-help-cards.tsx; after this commit those URLs resolve to missing files and the cards render with broken backgrounds in production. Please keep these assets (or update the component to use new asset paths) so the members onboarding UI does not regress.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/helpers/pages/admin/members/members-page.ts`:
- Around line 5-8: Make the locator visibility consistent by changing the
declarations of newMemberButton and memberListItems to public readonly so they
match loadMoreButton and membersListScrollRoot; update the class member
declarations for newMemberButton and memberListItems (the Locator fields) to use
public readonly so tests can access them for assertions without TS visibility
errors.
🪄 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: 52c4a02e-0230-4e9e-a86a-e0156dcf7a24
⛔ Files ignored due to path filters (4)
ghost/admin/public/assets/icons/email-member.svgis excluded by!**/*.svgghost/admin/public/assets/icons/import-in-progress.svgis excluded by!**/*.svgghost/admin/public/assets/icons/members-all.svgis excluded by!**/*.svgghost/admin/public/assets/icons/members-outline.svgis excluded by!**/*.svg
📒 Files selected for processing (90)
apps/admin/src/ember-bridge/ember-bridge.test.tsxapps/admin/src/ember-bridge/ember-bridge.tsxe2e/helpers/pages/admin/members/members-list-page.tse2e/helpers/pages/admin/members/members-page.tsghost/admin/app/components/gh-member-single-label-input.hbsghost/admin/app/components/gh-member-single-label-input.jsghost/admin/app/components/gh-members-import-mapping-input.hbsghost/admin/app/components/gh-members-import-mapping-input.jsghost/admin/app/components/gh-members-import-table.hbsghost/admin/app/components/gh-members-import-table.jsghost/admin/app/components/gh-members-no-members.hbsghost/admin/app/components/gh-members-no-members.jsghost/admin/app/components/members/filter-value.hbsghost/admin/app/components/members/filter-value.jsghost/admin/app/components/members/filter.hbsghost/admin/app/components/members/filter.jsghost/admin/app/components/members/filters/audience-feedback.jsghost/admin/app/components/members/filters/columns/date-column.jsghost/admin/app/components/members/filters/created-at.jsghost/admin/app/components/members/filters/email-clicked.jsghost/admin/app/components/members/filters/email-count.jsghost/admin/app/components/members/filters/email-open-rate.jsghost/admin/app/components/members/filters/email-opened-count.jsghost/admin/app/components/members/filters/email-opened.jsghost/admin/app/components/members/filters/email-sent.jsghost/admin/app/components/members/filters/email.jsghost/admin/app/components/members/filters/index.jsghost/admin/app/components/members/filters/label.jsghost/admin/app/components/members/filters/last-seen.jsghost/admin/app/components/members/filters/name.jsghost/admin/app/components/members/filters/next-billing-date.jsghost/admin/app/components/members/filters/offers.jsghost/admin/app/components/members/filters/plan-interval.jsghost/admin/app/components/members/filters/relation-options/contains.jsghost/admin/app/components/members/filters/relation-options/date.jsghost/admin/app/components/members/filters/relation-options/index.jsghost/admin/app/components/members/filters/relation-options/match.jsghost/admin/app/components/members/filters/relation-options/number.jsghost/admin/app/components/members/filters/signup-attribution.jsghost/admin/app/components/members/filters/status.jsghost/admin/app/components/members/filters/subscribed.jsghost/admin/app/components/members/filters/subscription-attribution.jsghost/admin/app/components/members/filters/subscription-start-date.jsghost/admin/app/components/members/filters/subscription-status.jsghost/admin/app/components/members/filters/tier.jsghost/admin/app/components/members/list-item-column.hbsghost/admin/app/components/members/list-item-column.jsghost/admin/app/components/members/list-item-loading.hbsghost/admin/app/components/members/list-item.hbsghost/admin/app/components/members/list-item.jsghost/admin/app/components/members/modals/bulk-add-label.hbsghost/admin/app/components/members/modals/bulk-add-label.jsghost/admin/app/components/members/modals/bulk-delete.hbsghost/admin/app/components/members/modals/bulk-delete.jsghost/admin/app/components/members/modals/bulk-remove-label.hbsghost/admin/app/components/members/modals/bulk-remove-label.jsghost/admin/app/components/members/modals/bulk-unsubscribe.hbsghost/admin/app/components/members/modals/bulk-unsubscribe.jsghost/admin/app/components/modal-import-members.hbsghost/admin/app/components/modal-import-members.jsghost/admin/app/components/modal-import-members/csv-file-mapping.hbsghost/admin/app/components/modal-import-members/csv-file-mapping.jsghost/admin/app/components/modal-import-members/csv-file-select.hbsghost/admin/app/components/modal-import-members/csv-file-select.jsghost/admin/app/components/modal-unsubscribe-members.hbsghost/admin/app/components/modal-unsubscribe-members.jsghost/admin/app/components/offers/segment-select.hbsghost/admin/app/components/offers/segment-select.jsghost/admin/app/components/tiers/segment-select.hbsghost/admin/app/components/tiers/segment-select.jsghost/admin/app/controllers/member.jsghost/admin/app/controllers/members.jsghost/admin/app/controllers/members/import.jsghost/admin/app/errors/member-import-error.jsghost/admin/app/helpers/reset-query-params.jsghost/admin/app/routes/member.jsghost/admin/app/routes/members.jsghost/admin/app/routes/members/import.jsghost/admin/app/services/member-import-validator.jsghost/admin/app/services/state-bridge.jsghost/admin/app/styles/app-dark.cssghost/admin/app/styles/app.cssghost/admin/app/styles/components/filter-builder.cssghost/admin/app/styles/layouts/dashboard.cssghost/admin/app/styles/layouts/members.cssghost/admin/app/templates/members.hbsghost/admin/app/templates/members/import.hbsghost/admin/mirage/config/members.jsghost/admin/mirage/routes-test.jsghost/admin/package.json
💤 Files with no reviewable changes (84)
- ghost/admin/app/routes/member.js
- ghost/admin/app/components/modal-import-members/csv-file-select.js
- ghost/admin/app/components/members/filters/offers.js
- ghost/admin/app/components/members/filters/plan-interval.js
- ghost/admin/app/components/members/filters/email-open-rate.js
- ghost/admin/app/components/members/filters/subscription-status.js
- ghost/admin/app/components/members/filters/relation-options/date.js
- ghost/admin/app/components/modal-unsubscribe-members.hbs
- ghost/admin/app/components/members/filters/email-opened.js
- ghost/admin/app/components/members/filters/relation-options/number.js
- ghost/admin/app/components/members/filters/relation-options/match.js
- ghost/admin/app/components/modal-unsubscribe-members.js
- ghost/admin/app/components/tiers/segment-select.hbs
- ghost/admin/app/components/members/filters/next-billing-date.js
- ghost/admin/app/components/members/list-item.hbs
- ghost/admin/app/components/members/filters/status.js
- ghost/admin/app/components/members/filters/tier.js
- ghost/admin/app/components/members/filters/email-count.js
- ghost/admin/app/errors/member-import-error.js
- ghost/admin/app/components/members/list-item-column.hbs
- ghost/admin/app/templates/members/import.hbs
- ghost/admin/app/components/members/list-item-loading.hbs
- ghost/admin/app/components/members/filters/created-at.js
- ghost/admin/app/components/members/filters/relation-options/index.js
- apps/admin/src/ember-bridge/ember-bridge.tsx
- ghost/admin/app/components/members/filter.hbs
- ghost/admin/app/components/gh-members-import-mapping-input.js
- ghost/admin/package.json
- ghost/admin/app/styles/components/filter-builder.css
- ghost/admin/app/components/members/filters/last-seen.js
- ghost/admin/app/routes/members/import.js
- ghost/admin/app/components/members/list-item.js
- ghost/admin/app/components/gh-member-single-label-input.hbs
- ghost/admin/app/components/members/modals/bulk-add-label.hbs
- ghost/admin/app/components/gh-members-import-mapping-input.hbs
- ghost/admin/app/components/members/filters/email.js
- ghost/admin/app/components/members/filters/email-sent.js
- ghost/admin/app/components/modal-import-members/csv-file-select.hbs
- ghost/admin/app/components/members/filters/name.js
- ghost/admin/app/controllers/members/import.js
- ghost/admin/app/components/members/filters/columns/date-column.js
- ghost/admin/app/components/members/modals/bulk-delete.js
- ghost/admin/app/components/members/filter-value.hbs
- ghost/admin/app/components/members/filters/subscription-start-date.js
- ghost/admin/app/components/gh-members-no-members.hbs
- apps/admin/src/ember-bridge/ember-bridge.test.tsx
- ghost/admin/app/components/members/modals/bulk-remove-label.hbs
- ghost/admin/app/components/members/filter-value.js
- ghost/admin/app/templates/members.hbs
- ghost/admin/app/components/members/modals/bulk-remove-label.js
- ghost/admin/app/components/modal-import-members/csv-file-mapping.js
- ghost/admin/app/components/members/filters/signup-attribution.js
- ghost/admin/app/styles/app-dark.css
- ghost/admin/app/components/members/filters/subscription-attribution.js
- ghost/admin/app/components/members/modals/bulk-unsubscribe.js
- ghost/admin/app/controllers/members.js
- ghost/admin/app/components/members/filters/subscribed.js
- ghost/admin/app/components/members/filters/email-clicked.js
- ghost/admin/app/components/members/modals/bulk-add-label.js
- ghost/admin/app/components/gh-members-import-table.hbs
- ghost/admin/app/components/modal-import-members/csv-file-mapping.hbs
- ghost/admin/app/components/members/filters/email-opened-count.js
- ghost/admin/app/components/members/filters/index.js
- ghost/admin/app/components/members/list-item-column.js
- ghost/admin/app/components/members/filter.js
- ghost/admin/mirage/config/members.js
- ghost/admin/app/components/members/modals/bulk-unsubscribe.hbs
- ghost/admin/app/components/members/modals/bulk-delete.hbs
- ghost/admin/app/components/gh-members-no-members.js
- ghost/admin/app/components/gh-members-import-table.js
- ghost/admin/app/components/members/filters/audience-feedback.js
- ghost/admin/app/components/offers/segment-select.hbs
- ghost/admin/app/styles/layouts/dashboard.css
- ghost/admin/app/components/modal-import-members.hbs
- ghost/admin/app/components/offers/segment-select.js
- ghost/admin/app/helpers/reset-query-params.js
- ghost/admin/app/routes/members.js
- ghost/admin/app/styles/app.css
- ghost/admin/app/components/gh-member-single-label-input.js
- ghost/admin/app/components/tiers/segment-select.js
- ghost/admin/app/components/members/filters/relation-options/contains.js
- ghost/admin/app/components/members/filters/label.js
- ghost/admin/app/components/modal-import-members.js
- ghost/admin/app/services/member-import-validator.js
✅ Files skipped from review due to trivial changes (2)
- ghost/admin/app/services/state-bridge.js
- e2e/helpers/pages/admin/members/members-list-page.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/admin/app/controllers/member.js
Comment on lines
5
to
8
| readonly newMemberButton: Locator; | ||
| public readonly loadMoreButton: Locator; | ||
| public readonly membersListScrollRoot: Locator; | ||
| readonly memberListItems: Locator; |
Contributor
There was a problem hiding this comment.
Inconsistent public readonly on locators used externally.
newMemberButton (line 5) and memberListItems (line 8) are declared as just readonly, while loadMoreButton and membersListScrollRoot (lines 6–7) are public readonly. Per the coding guidelines, locators used with assertions must be public readonly. The inconsistency means any test directly asserting against newMemberButton or memberListItems would get a TypeScript private visibility error.
♻️ Proposed fix
- readonly newMemberButton: Locator;
+ public readonly newMemberButton: Locator;
public readonly loadMoreButton: Locator;
public readonly membersListScrollRoot: Locator;
- readonly memberListItems: Locator;
+ public readonly memberListItems: Locator;As per coding guidelines: "Page Objects should expose locators as public readonly when used with assertions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/helpers/pages/admin/members/members-page.ts` around lines 5 - 8, Make the
locator visibility consistent by changing the declarations of newMemberButton
and memberListItems to public readonly so they match loadMoreButton and
membersListScrollRoot; update the class member declarations for newMemberButton
and memberListItems (the Locator fields) to use public readonly so tests can
access them for assertions without TS visibility errors.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin/src/ember-bridge/ember-bridge.test.tsx (1)
140-168: 💤 Low valueConsider adding
unmount()for consistency with other tests.Other similar tests in this file (e.g., lines 114, 233) call
unmount()after assertions. While the test will still pass without it, adding the unmount call would maintain consistency with the test suite patterns.♻️ Suggested fix
await waitFor(() => { const queries = queryClient.getQueryCache().getAll(); const changedQueries = queries.filter(q => ['MembersResponseType', 'CommentsResponseType'].includes(q.queryKey[0] as string)); const unchangedQueries = queries.filter(q => !['MembersResponseType', 'CommentsResponseType'].includes(q.queryKey[0] as string)); expect(changedQueries.every(q => q.state.isInvalidated)).toBe(true); expect(unchangedQueries.every(q => !q.state.isInvalidated)).toBe(true); }); + + unmount(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/ember-bridge/ember-bridge.test.tsx` around lines 140 - 168, Add a call to the rendered hook's unmount() for consistency: after the final assertions in the "invalidates member and comment queries when member commenting changes" test (where you call renderHook(() => useEmberDataSync(), { wrapper })) capture the return value (which provides unmount) and invoke unmount() at the end of the test to mirror other tests that call unmount() and ensure cleanup of the hook instance created by renderHook.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/admin/src/ember-bridge/ember-bridge.test.tsx`:
- Around line 140-168: Add a call to the rendered hook's unmount() for
consistency: after the final assertions in the "invalidates member and comment
queries when member commenting changes" test (where you call renderHook(() =>
useEmberDataSync(), { wrapper })) capture the return value (which provides
unmount) and invoke unmount() at the end of the test to mirror other tests that
call unmount() and ensure cleanup of the hook instance created by renderHook.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7eb5c3dd-eaa2-43d7-9c1b-67e7acbd4a8d
📒 Files selected for processing (7)
apps/admin/src/ember-bridge/ember-bridge.test.tsxapps/admin/src/ember-bridge/ember-bridge.tsxghost/admin/app/components/members/modals/disable-commenting.jsghost/admin/app/controllers/member.jsghost/admin/app/services/state-bridge.jsghost/admin/tests/unit/controllers/member-test.jsghost/admin/tests/unit/services/state-bridge-test.js
💤 Files with no reviewable changes (1)
- ghost/admin/app/components/members/modals/disable-commenting.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/admin/tests/unit/controllers/member-test.js
ref https://linear.app/ghost/issue/BER-3580/remove-ember-members-implementation Member commenting changes need to refresh both React member and comment data, but Ember UI should not call the AdminX query client or know React query data type details directly.
jonatansberg
force-pushed
the
jonatan-ber-3580-remove-ember-members-implementation
branch
from
251d1ab to
84f4b8f
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/TryGhost/Ghost/blob/84f4b8fb5c4ff2f0796adf7a05bb282f58507b5a/ghost/admin/public/assets/img/marketing/members-1.jpg#L1
Deleting these marketing JPEGs breaks the React members empty-state help cards, which still hardcode assetRoot + "img/marketing/members-1.jpg" and members-2.jpg in apps/posts/src/views/members/components/members-help-cards.tsx. After this commit, opening the members empty state shows broken image panels for users because no replacement assets are provided.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ref https://linear.app/ghost/issue/BER-3580/remove-ember-members-implementation The retained Ember member detail and activity screens no longer reference these styles after the list, import, and older member UI paths were removed.
ref https://linear.app/ghost/issue/BER-3580/remove-ember-members-implementation The extra cleanup was outside the bridge follow-up scope, so this restores the PR to the bridge-focused changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes the old Ember members list, filters, import/export UI, and their tests.
/membersand/members/importare now handled by React.The Ember member detail/new/activity screens stay in place. Member detail invalidates the React members cache through the existing Ember bridge when member data changes.
Reviewer note
GitHub makes this look larger than it is because
members.csshad a large block removed near the top, so the default diff shows retained CSS as newly added.+444/-1586+14/-1156This PR is mostly deletion, with only a small amount of real new code.
Verification
git diff --checkpnpm --filter @tryghost/admin test:unitpnpm --filter ghost-admin lintpnpm exec ember exam --split 1 --parallel 1 --filter "Unit: Controller: member"