BER-3361: Remove members-forward release gate#27221
Open
jonatansberg wants to merge 14 commits intomainfrom
Open
BER-3361: Remove members-forward release gate#27221jonatansberg wants to merge 14 commits intomainfrom
jonatansberg wants to merge 14 commits intomainfrom
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:
WalkthroughRemoved the Possibly related PRs
Suggested reviewers
🚥 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 |
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24124282464 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24127132919 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #27221 +/- ##
==========================================
- Coverage 73.42% 72.89% -0.54%
==========================================
Files 1552 1553 +1
Lines 124677 124914 +237
Branches 15073 15100 +27
==========================================
- Hits 91544 91055 -489
- Misses 32126 32904 +778
+ Partials 1007 955 -52
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:
|
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24129099035 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24181947096 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
![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: 3
🧹 Nitpick comments (5)
ghost/admin/tests/acceptance/members-test.js (1)
11-11: Avoid disabling the entire acceptance suite withdescribe.skip.Line 11 currently skips every members acceptance test in this file, which drops CI coverage for still-supported
/membersbehavior and can hide regressions. Please keep the suite enabled and only skip/remove truly obsolete cases.Proposed minimal fix
-describe.skip('Acceptance: Members Test', function () { +describe('Acceptance: Members Test', function () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/tests/acceptance/members-test.js` at line 11, The test suite is entirely disabled by using describe.skip for the 'Acceptance: Members Test' suite; change describe.skip to describe to re-enable the acceptance tests in members-test.js, and if certain individual specs are truly obsolete mark those with it.skip or remove them instead of skipping the whole suite (locate the top-level describe.skip('Acceptance: Members Test', ...) and update it accordingly).e2e/tests/admin/members/export.test.ts (1)
96-105: Extract repeated export metadata assertions into a shared helper.Both scenarios repeat the same assertions for IDs/timestamps and filename shape. Pulling that into one helper will reduce drift as export behavior evolves.
♻️ Optional refactor
+function assertExportMetadata( + suggestedFilename: string, + contentIds: RegExpMatchArray | null, + contentTimestamps: RegExpMatchArray | null, + expectedCount: number +) { + expect(contentIds).toHaveLength(expectedCount); + expect(contentTimestamps).toHaveLength(expectedCount); + expect(suggestedFilename.startsWith('members')).toBe(true); + expect(suggestedFilename.endsWith('.csv')).toBe(true); +} + test.describe('Ghost Admin - Members Export', () => { @@ - expect(contentIds).toHaveLength(MEMBERS_FIXTURE.length); - expect(contentTimestamps).toHaveLength(MEMBERS_FIXTURE.length); - expect(suggestedFilename.startsWith('members')).toBe(true); - expect(suggestedFilename.endsWith('.csv')).toBe(true); + assertExportMetadata(suggestedFilename, contentIds, contentTimestamps, MEMBERS_FIXTURE.length); @@ - expect(contentIds).toHaveLength(filteredMembers.length); - expect(contentTimestamps).toHaveLength(filteredMembers.length); - expect(suggestedFilename.startsWith('members')).toBe(true); - expect(suggestedFilename.endsWith('.csv')).toBe(true); + assertExportMetadata(suggestedFilename, contentIds, contentTimestamps, filteredMembers.length);Also applies to: 119-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/members/export.test.ts` around lines 96 - 105, Create a shared test helper that centralizes the repeated export assertions: move the checks referencing membersPage.exportMembers() results (suggestedFilename, content), extractDownloadedContentSpecifics(content) outputs (contentIds, contentTimestamps), and assertExportedMembers(content, MEMBERS_FIXTURE) into one function (e.g., assertExportedMembersMetadata or verifyExportDownload) and call it from both test sites; the helper should assert contentIds and contentTimestamps lengths equal MEMBERS_FIXTURE.length and that suggestedFilename startsWith 'members' and endsWith '.csv', then replace the duplicated assertions in the tests with a single call to this new helper.apps/admin/src/layout/app-sidebar/nav-content.tsx (2)
80-97: Extract the members base path to one constant.
'members'is now hardcoded in both theuseIsActiveLinkcall and the nav target. Pulling that into a shared constant will make future route cleanup less error-prone.♻️ Suggested cleanup
+const MEMBERS_ROUTE = 'members'; const LEGACY_MEMBERS_ACTIVE_ROUTES = ['member', 'member.new', 'members-activity']; ... -const isMembersRouteActive = useIsActiveLink({path: 'members', activeOnSubpath: true}); +const isMembersRouteActive = useIsActiveLink({path: MEMBERS_ROUTE, activeOnSubpath: true}); ... -const membersRoute = 'members'; +const membersRoute = MEMBERS_ROUTE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/layout/app-sidebar/nav-content.tsx` around lines 80 - 97, Extract the hardcoded 'members' string into a single constant (e.g., MEMBERS_ROUTE) and use that constant wherever the literal is used: replace the argument in useIsActiveLink({path: 'members', ...}) and the membersRoute assignment (currently membersRoute = 'members'), and any other occurrences in this file (e.g., references involving isMembersRouteActive or membersNavActive) so all member-route logic references the new MEMBERS_ROUTE constant consistently.
91-93: Please add regression coverage for the legacy active-route fallback.This branch is now what keeps the Members item highlighted on Ember-only detail/new/activity screens after the
/members-forwardremoval. A focused test here would make the follow-up cleanup much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/layout/app-sidebar/nav-content.tsx` around lines 91 - 93, Add a focused regression test for the legacy active-route fallback used by membersNavActive: exercise the branch where isMembersRouteActive is false so the code falls back to routing.isRouteActive(LEGACY_MEMBERS_ACTIVE_ROUTES), and assert the Members item remains highlighted when routing.isRouteActive returns true; also include cases where isMembersRouteActive is true but either hasActiveMemberView or membersExpanded cause the expression to resolve false to ensure the original Ember-only detail/new/activity screens remain covered. Mock or stub routing.isRouteActive and render the component (or call the selector) to assert expected boolean outcomes for membersNavActive across these permutations.ghost/admin/app/routes/member.js (1)
57-65: Truncated comment at line 57.The cleanup logic is correct. However, the comment "Make sure we clear" appears to be incomplete. Consider completing it to explain what's being cleared and why.
📝 Suggested comment improvement
- // Make sure we clear + // Make sure we clear navigation state when leaving the member route if (isExiting) { controller.set('backPath', null); controller.set('directlyFromAnalytics', false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/app/routes/member.js` around lines 57 - 65, The comment "Make sure we clear" is incomplete—update it to briefly explain what and why is being cleared: when isExiting is true, we're resetting transient navigation state on the controller (clearing backPath and directlyFromAnalytics) and clearing postAnalytics if present to avoid leaking state between member route visits; modify the comment above the conditional that references isExiting and controller.postAnalytics (the block that sets backPath, directlyFromAnalytics, and postAnalytics to null/false) to a concise sentence like "Clear transient navigation and analytics state when exiting the route to avoid carrying state into the next visit."
🤖 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/controllers/member.js`:
- Around line 54-56: The current check this.backPath?.startsWith('/members') is
too permissive and can match unintended routes like '/members-forward'; tighten
validation by matching only the exact /members route or subpaths (e.g. use a
strict test like /^\/members(?:$|\/)/ or normalize and compare path segments)
before returning this.backPath so stale or manipulated back links cannot fall
through; update the conditional around this.backPath to use that stricter
pattern (reference: this.backPath and the startsWith check).
In `@ghost/admin/tests/acceptance/members/filter-test.js`:
- Line 16: The test suite is entirely skipped via describe.skip in the
Acceptance: Members filtering suite; remove or replace describe.skip in the
members/filter-test.js so the suite runs, then run the tests and either fix
failing tests (adapting selectors, test setup, or stubs referenced inside the
describe block) or, if a specific case truly must be skipped, change only that
individual it(...) to it.skip and add a linked follow-up issue ID in the test’s
comment; ensure you reference the top-level describe('Acceptance: Members
filtering', ...) and any failing nested tests when making fixes.
In `@ghost/admin/tests/acceptance/members/import-test.js`:
- Line 11: The test suite has been disabled via describe.skip in the Members
import acceptance test (describe.skip in members/import-test.js), which silences
all coverage; either re-enable the suite by changing describe.skip back to
describe so tests run, or if the suite is truly obsolete remove the entire
describe block; if re-enabling, update any assertions and route references
inside the suite (e.g., route transition expectations and controller/route
ownership checks within the tests) to match the new route ownership and paths so
the restored tests pass against the current implementation.
---
Nitpick comments:
In `@apps/admin/src/layout/app-sidebar/nav-content.tsx`:
- Around line 80-97: Extract the hardcoded 'members' string into a single
constant (e.g., MEMBERS_ROUTE) and use that constant wherever the literal is
used: replace the argument in useIsActiveLink({path: 'members', ...}) and the
membersRoute assignment (currently membersRoute = 'members'), and any other
occurrences in this file (e.g., references involving isMembersRouteActive or
membersNavActive) so all member-route logic references the new MEMBERS_ROUTE
constant consistently.
- Around line 91-93: Add a focused regression test for the legacy active-route
fallback used by membersNavActive: exercise the branch where
isMembersRouteActive is false so the code falls back to
routing.isRouteActive(LEGACY_MEMBERS_ACTIVE_ROUTES), and assert the Members item
remains highlighted when routing.isRouteActive returns true; also include cases
where isMembersRouteActive is true but either hasActiveMemberView or
membersExpanded cause the expression to resolve false to ensure the original
Ember-only detail/new/activity screens remain covered. Mock or stub
routing.isRouteActive and render the component (or call the selector) to assert
expected boolean outcomes for membersNavActive across these permutations.
In `@e2e/tests/admin/members/export.test.ts`:
- Around line 96-105: Create a shared test helper that centralizes the repeated
export assertions: move the checks referencing membersPage.exportMembers()
results (suggestedFilename, content), extractDownloadedContentSpecifics(content)
outputs (contentIds, contentTimestamps), and assertExportedMembers(content,
MEMBERS_FIXTURE) into one function (e.g., assertExportedMembersMetadata or
verifyExportDownload) and call it from both test sites; the helper should assert
contentIds and contentTimestamps lengths equal MEMBERS_FIXTURE.length and that
suggestedFilename startsWith 'members' and endsWith '.csv', then replace the
duplicated assertions in the tests with a single call to this new helper.
In `@ghost/admin/app/routes/member.js`:
- Around line 57-65: The comment "Make sure we clear" is incomplete—update it to
briefly explain what and why is being cleared: when isExiting is true, we're
resetting transient navigation state on the controller (clearing backPath and
directlyFromAnalytics) and clearing postAnalytics if present to avoid leaking
state between member route visits; modify the comment above the conditional that
references isExiting and controller.postAnalytics (the block that sets backPath,
directlyFromAnalytics, and postAnalytics to null/false) to a concise sentence
like "Clear transient navigation and analytics state when exiting the route to
avoid carrying state into the next visit."
In `@ghost/admin/tests/acceptance/members-test.js`:
- Line 11: The test suite is entirely disabled by using describe.skip for the
'Acceptance: Members Test' suite; change describe.skip to describe to re-enable
the acceptance tests in members-test.js, and if certain individual specs are
truly obsolete mark those with it.skip or remove them instead of skipping the
whole suite (locate the top-level describe.skip('Acceptance: Members Test', ...)
and update it accordingly).
🪄 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: e61c4aa8-7850-4d7f-b1e9-5ea6a1d99938
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (44)
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxapps/admin/src/layout/app-sidebar/nav-content.tsxapps/admin/src/members-route-gate.tsxapps/admin/src/members-route.test.tsxapps/admin/src/members-route.tsxapps/admin/src/routes.tsxapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/components/members-list-item.tsxapps/posts/src/views/members/components/members-list.tsxapps/posts/src/views/members/member-detail-hash.tsapps/posts/src/views/members/members.tsxapps/posts/test/unit/views/members/members-actions.test.tsxapps/posts/test/unit/views/members/members-list.test.tse2e/helpers/pages/admin/members/member-details-page.tse2e/tests/admin/analytics/post-analytics/growth.test.tse2e/tests/admin/members-legacy/disable-commenting.test.tse2e/tests/admin/members-legacy/filter-actions.test.tse2e/tests/admin/members-legacy/impersonation.test.tse2e/tests/admin/members-legacy/import.test.tse2e/tests/admin/members-legacy/member-activity-events.test.tse2e/tests/admin/members-legacy/members.test.tse2e/tests/admin/members-legacy/stripe-webhooks.test.tse2e/tests/admin/members/bulk-actions.test.tse2e/tests/admin/members/export.test.tse2e/tests/admin/members/list.test.tse2e/tests/admin/members/multiselect-filters.test.tse2e/tests/admin/members/saved-views.test.tse2e/tests/admin/members/search-and-filter.test.tse2e/tests/admin/members/tier-filter-search.test.tse2e/tests/admin/members/virtual-window.test.tsghost/admin/app/components/dashboard/onboarding-checklist.hbsghost/admin/app/components/editor/publish-options/publish-type.hbsghost/admin/app/components/posts-list/list-item.hbsghost/admin/app/controllers/member.jsghost/admin/app/router.jsghost/admin/app/routes/member.jsghost/admin/app/routes/members.jsghost/admin/app/services/feature.jsghost/admin/app/templates/member.hbsghost/admin/tests/acceptance/authentication-test.jsghost/admin/tests/acceptance/members-test.jsghost/admin/tests/acceptance/members/filter-test.jsghost/admin/tests/acceptance/members/import-test.jsghost/core/core/shared/labs.js
💤 Files with no reviewable changes (18)
- e2e/tests/admin/members/saved-views.test.ts
- e2e/tests/admin/members/search-and-filter.test.ts
- apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx
- e2e/tests/admin/members/virtual-window.test.ts
- apps/admin/src/members-route-gate.tsx
- ghost/admin/app/router.js
- e2e/tests/admin/members-legacy/impersonation.test.ts
- ghost/admin/app/routes/members.js
- e2e/tests/admin/members/tier-filter-search.test.ts
- apps/admin/src/routes.tsx
- ghost/admin/app/services/feature.js
- e2e/tests/admin/members-legacy/disable-commenting.test.ts
- e2e/tests/admin/members-legacy/members.test.ts
- e2e/tests/admin/members-legacy/import.test.ts
- e2e/tests/admin/members-legacy/filter-actions.test.ts
- e2e/tests/admin/members/bulk-actions.test.ts
- ghost/core/core/shared/labs.js
- e2e/tests/admin/members-legacy/member-activity-events.test.ts
Comment on lines
+54
to
+56
| if (this.backPath?.startsWith('/members')) { | ||
| return this.backPath; | ||
| } |
Contributor
There was a problem hiding this comment.
Tighten backPath validation to avoid invalid route fallthrough.
Line 54 uses startsWith('/members'), which also accepts removed/non-list paths like /members-forward.... That can send delete/breadcrumb navigation to stale routes if back is manipulated or old links are used.
Suggested fix
- if (this.backPath?.startsWith('/members')) {
+ if (this.backPath?.match(/^\/members(?:$|\?)/)) {
return this.backPath;
}🤖 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 54 - 56, The current
check this.backPath?.startsWith('/members') is too permissive and can match
unintended routes like '/members-forward'; tighten validation by matching only
the exact /members route or subpaths (e.g. use a strict test like
/^\/members(?:$|\/)/ or normalize and compare path segments) before returning
this.backPath so stale or manipulated back links cannot fall through; update the
conditional around this.backPath to use that stricter pattern (reference:
this.backPath and the startsWith check).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/admin/src/layout/app-sidebar/nav-content.tsx (1)
80-80: Optional: centralize the"members"route literal.Using one constant for the route path would reduce drift risk between active-state checks and link targets.
♻️ Suggested refactor
+const MEMBERS_ROUTE_PATH = 'members'; const LEGACY_MEMBERS_ACTIVE_ROUTES = ['member', 'member.new', 'members-activity']; -const isMembersRouteActive = useIsActiveLink({path: 'members', activeOnSubpath: true}); +const isMembersRouteActive = useIsActiveLink({path: MEMBERS_ROUTE_PATH, activeOnSubpath: true}); ... - to="members" + to={MEMBERS_ROUTE_PATH} ... - to="members" + to={MEMBERS_ROUTE_PATH}Also applies to: 183-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/layout/app-sidebar/nav-content.tsx` at line 80, The "members" route string is repeated (e.g., in the isMembersRouteActive useIsActiveLink call and elsewhere around lines 183-197), which risks drift; extract that literal into a single constant (e.g., MEMBERS_ROUTE or MEMBERS_PATH) and replace all occurrences where the route is referenced (for active-state checks like isMembersRouteActive and any Link/Href targets) so both use the same identifier; update import/exports if needed so the constant is shared where used.
🤖 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/layout/app-sidebar/nav-content.tsx`:
- Line 80: The "members" route string is repeated (e.g., in the
isMembersRouteActive useIsActiveLink call and elsewhere around lines 183-197),
which risks drift; extract that literal into a single constant (e.g.,
MEMBERS_ROUTE or MEMBERS_PATH) and replace all occurrences where the route is
referenced (for active-state checks like isMembersRouteActive and any Link/Href
targets) so both use the same identifier; update import/exports if needed so the
constant is shared where used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 115fe09e-99b1-4d52-8702-3a5d5594054d
📒 Files selected for processing (6)
apps/admin/src/layout/app-sidebar/nav-content.tsxapps/posts/src/views/members/components/members-list-item.tsxapps/posts/src/views/members/components/members-list.tsxapps/posts/src/views/members/member-detail-hash.tsapps/posts/test/unit/views/members/members-list.test.tse2e/tests/admin/members/list.test.ts
✅ Files skipped from review due to trivial changes (3)
- apps/posts/test/unit/views/members/members-list.test.ts
- apps/posts/src/views/members/member-detail-hash.ts
- apps/posts/src/views/members/components/members-list.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/tests/admin/members/import.test.ts (1)
16-31: Optional: clean up temporary CSV files after test execution.The temp file created around Line 29–Line 30 is never removed. Wrapping import steps in
try/finallywithunlinkSync(csvPath)would avoid local/CI temp-dir buildup over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/members/import.test.ts` around lines 16 - 31, The test creates a temp CSV via tmpdir() and writeFileSync into csvPath but never deletes it; wrap the import and assertions that use csvPath (the block around the import steps in import.test.ts) in a try/finally and call unlinkSync(csvPath) in the finally to ensure cleanup; reference csvPath, writeFileSync and tmpdir() to locate the creation and add unlinkSync(csvPath) to the finally block so temp files are removed even if the test fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/admin/members/import.test.ts`:
- Around line 16-31: The test creates a temp CSV via tmpdir() and writeFileSync
into csvPath but never deletes it; wrap the import and assertions that use
csvPath (the block around the import steps in import.test.ts) in a try/finally
and call unlinkSync(csvPath) in the finally to ensure cleanup; reference
csvPath, writeFileSync and tmpdir() to locate the creation and add
unlinkSync(csvPath) to the finally block so temp files are removed even if the
test fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 661ddbaa-1ca4-4f55-b780-933798c60b03
📒 Files selected for processing (3)
e2e/tests/admin/members-legacy/members.test.tse2e/tests/admin/members/bulk-actions.test.tse2e/tests/admin/members/import.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/tests/admin/members-legacy/members.test.ts
- e2e/tests/admin/members/bulk-actions.test.ts
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24190425447 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release This releases the React-owned /members routes while keeping Ember detail flows intact and deferring dead-code cleanup to the next stacked PR.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release This aligns the import-route unit coverage with the released behavior after removing the members-forward flag.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release This drops the extra routes test file so the release branch stays focused on the shipped members routing change.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release The config and runtime changes already cover the flag removal, so this extra assertion only added noise to the branch.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release Reused the shared link active matcher for the React-owned members routes and dropped the bespoke pathname check.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release The release branch moved /members out of the Ember test lane, so the auth acceptance tests now use an Ember-owned protected route and the analytics assertion matches the released React copy.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release The old filter-actions Playwright spec still drove Ember-only members filter controls that no longer exist on the released React /members route, while equivalent bulk-label coverage already exists in the React members suite.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release These Playwright tests still asserted the old Ember members list surface even though /members is now React-owned, and the retained Ember detail flows are covered by the remaining detail-focused specs.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release Row clicks on the React members list were bypassing the back query string even though the rendered links already carried it. This shares one detail-hash builder between link and row navigation so retained Ember detail screens can return to the filtered list state.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release The release branch still ran Ember-list import and export coverage even though those routes are no longer part of the shipped members list. This drops the dead import spec and keeps the export coverage focused on the React-owned members route.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release Removed redundant nav wiring, simplified the member detail path helper naming, and dropped the stale members-route e2e case without changing runtime behavior.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release Migrated the dropped React import flow and retained legacy member-detail coverage, and tightened bulk-action coverage so the filtered-members behavior still has an active browser test.
ref https://linear.app/ghost/issue/BER-3361/remove-feature-flag-release Updated the Playwright import modal helper to match the shipped React import dialog instead of the old Ember data-test selectors so the restored browser coverage exercises the real UI.
rob-ghost
force-pushed
the
ber-3361-remove-feature-flag-release
branch
from
428d463 to
13f4ef3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/helpers/pages/admin/members/members-import-modal.ts (1)
21-24: Scope mapping row lookup to the modal to avoid cross-page row collisions.Line 22 still queries from
this.page, so rows outside the modal can be matched in noisy admin screens. Prefer keeping this scoped tothis.dialogfor consistency with the new locator strategy.Suggested refactor
getMappingRow(fieldName: string): Locator { - return this.page.getByRole('row').filter({ - has: this.page.getByRole('cell', {name: fieldName, exact: true}) + return this.dialog.getByRole('row').filter({ + has: this.dialog.getByRole('cell', {name: fieldName, exact: true}) }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/members/members-import-modal.ts` around lines 21 - 24, The getMappingRow function currently queries rows from this.page which can match elements outside the modal; change both role lookups to be scoped to the modal by using this.dialog.getByRole(...) for the row and the cell (i.e., replace this.page.getByRole with this.dialog.getByRole inside getMappingRow) so the locator only matches rows inside the dialog and still returns a Locator from getMappingRow.apps/admin/src/layout/app-sidebar/nav-content.tsx (1)
89-93: Consider naming the activation branches for readability.This logic is correct, but the ternary is a little dense. Extracting intermediate booleans would make future maintenance safer.
♻️ Optional readability refactor
- const membersNavActive = isMembersRouteActive - ? (!hasActiveMemberView || !membersExpanded) - : routing.isRouteActive(LEGACY_MEMBERS_ACTIVE_ROUTES); + const shouldHighlightMembersBase = !hasActiveMemberView || !membersExpanded; + const isLegacyMembersRouteActive = routing.isRouteActive(LEGACY_MEMBERS_ACTIVE_ROUTES); + const membersNavActive = isMembersRouteActive + ? shouldHighlightMembersBase + : isLegacyMembersRouteActive;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/layout/app-sidebar/nav-content.tsx` around lines 89 - 93, The ternary that computes membersNavActive is dense; introduce descriptive intermediate booleans to name the branches (e.g., activeRouteHandlesMemberViews = isMembersRouteActive and hasActiveMemberView && membersExpanded, or legacyRouteActive = routing.isRouteActive(LEGACY_MEMBERS_ACTIVE_ROUTES)) and then set membersNavActive using those names so the intent of the two branches is explicit; update references to hasActiveMemberView, membersExpanded (savedMembersExpanded), isMembersRouteActive, routing.isRouteActive and LEGACY_MEMBERS_ACTIVE_ROUTES accordingly.
🤖 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/tests/admin/members/list.test.ts`:
- Line 55: Rename the test title string in the test declaration test('preserves
filters when returning from member detail', async ({page}) => { ... }) to follow
the required lowercase "what is tested - expected outcome" format; change it to
test('returning from member detail - preserves applied filters', async ({page})
=> { ... }) so the test name matches guidelines.
---
Nitpick comments:
In `@apps/admin/src/layout/app-sidebar/nav-content.tsx`:
- Around line 89-93: The ternary that computes membersNavActive is dense;
introduce descriptive intermediate booleans to name the branches (e.g.,
activeRouteHandlesMemberViews = isMembersRouteActive and hasActiveMemberView &&
membersExpanded, or legacyRouteActive =
routing.isRouteActive(LEGACY_MEMBERS_ACTIVE_ROUTES)) and then set
membersNavActive using those names so the intent of the two branches is
explicit; update references to hasActiveMemberView, membersExpanded
(savedMembersExpanded), isMembersRouteActive, routing.isRouteActive and
LEGACY_MEMBERS_ACTIVE_ROUTES accordingly.
In `@e2e/helpers/pages/admin/members/members-import-modal.ts`:
- Around line 21-24: The getMappingRow function currently queries rows from
this.page which can match elements outside the modal; change both role lookups
to be scoped to the modal by using this.dialog.getByRole(...) for the row and
the cell (i.e., replace this.page.getByRole with this.dialog.getByRole inside
getMappingRow) so the locator only matches rows inside the dialog and still
returns a Locator from getMappingRow.
🪄 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: abf46348-39c5-4bcb-a6bc-27f789d717ef
📒 Files selected for processing (44)
apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsxapps/admin/src/layout/app-sidebar/nav-content.tsxapps/admin/src/members-route-gate.tsxapps/admin/src/members-route.test.tsxapps/admin/src/members-route.tsxapps/admin/src/routes.tsxapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/components/members-list-item.tsxapps/posts/src/views/members/components/members-list.tsxapps/posts/src/views/members/member-detail-hash.tsapps/posts/src/views/members/members.tsxapps/posts/test/unit/views/members/members-actions.test.tsxapps/posts/test/unit/views/members/members-list.test.tse2e/helpers/pages/admin/members/member-details-page.tse2e/helpers/pages/admin/members/members-import-modal.tse2e/tests/admin/analytics/post-analytics/growth.test.tse2e/tests/admin/members-legacy/disable-commenting.test.tse2e/tests/admin/members-legacy/filter-actions.test.tse2e/tests/admin/members-legacy/impersonation.test.tse2e/tests/admin/members-legacy/member-activity-events.test.tse2e/tests/admin/members-legacy/members.test.tse2e/tests/admin/members-legacy/stripe-webhooks.test.tse2e/tests/admin/members/bulk-actions.test.tse2e/tests/admin/members/export.test.tse2e/tests/admin/members/import.test.tse2e/tests/admin/members/list.test.tse2e/tests/admin/members/multiselect-filters.test.tse2e/tests/admin/members/saved-views.test.tse2e/tests/admin/members/search-and-filter.test.tse2e/tests/admin/members/tier-filter-search.test.tse2e/tests/admin/members/virtual-window.test.tsghost/admin/app/components/dashboard/onboarding-checklist.hbsghost/admin/app/components/editor/publish-options/publish-type.hbsghost/admin/app/components/posts-list/list-item.hbsghost/admin/app/controllers/member.jsghost/admin/app/router.jsghost/admin/app/routes/member.jsghost/admin/app/routes/members.jsghost/admin/app/services/feature.jsghost/admin/app/templates/member.hbsghost/admin/tests/acceptance/authentication-test.jsghost/admin/tests/acceptance/members-test.jsghost/admin/tests/acceptance/members/filter-test.jsghost/admin/tests/acceptance/members/import-test.js
💤 Files with no reviewable changes (14)
- e2e/tests/admin/members-legacy/member-activity-events.test.ts
- e2e/tests/admin/members/search-and-filter.test.ts
- e2e/tests/admin/members/virtual-window.test.ts
- e2e/tests/admin/members/tier-filter-search.test.ts
- e2e/tests/admin/members/saved-views.test.ts
- ghost/admin/app/services/feature.js
- ghost/admin/app/router.js
- ghost/admin/app/routes/members.js
- apps/admin-x-settings/src/components/settings/advanced/labs/private-features.tsx
- apps/admin/src/members-route-gate.tsx
- e2e/tests/admin/members-legacy/disable-commenting.test.ts
- e2e/tests/admin/members-legacy/impersonation.test.ts
- apps/admin/src/routes.tsx
- e2e/tests/admin/members-legacy/filter-actions.test.ts
✅ Files skipped from review due to trivial changes (9)
- ghost/admin/tests/acceptance/members-test.js
- ghost/admin/tests/acceptance/members/import-test.js
- e2e/tests/admin/members-legacy/stripe-webhooks.test.ts
- e2e/tests/admin/analytics/post-analytics/growth.test.ts
- ghost/admin/tests/acceptance/members/filter-test.js
- e2e/tests/admin/members/multiselect-filters.test.ts
- apps/posts/src/views/members/member-detail-hash.ts
- apps/posts/test/unit/views/members/members-actions.test.tsx
- apps/posts/src/views/members/components/members-list-item.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/posts/src/views/members/members.tsx
- apps/admin/src/members-route.tsx
- ghost/admin/app/components/posts-list/list-item.hbs
- apps/posts/test/unit/views/members/members-list.test.ts
- e2e/tests/admin/members/bulk-actions.test.ts
- e2e/helpers/pages/admin/members/member-details-page.ts
- ghost/admin/app/components/editor/publish-options/publish-type.hbs
- ghost/admin/app/templates/member.hbs
- apps/posts/src/views/members/components/members-list.tsx
- apps/admin/src/members-route.test.tsx
- e2e/tests/admin/members/export.test.ts
- e2e/tests/admin/members/import.test.ts
- e2e/tests/admin/members-legacy/members.test.ts
| await expect(page).toHaveURL(new RegExp(`/members/${member.id}`)); | ||
| }); | ||
|
|
||
| test('preserves filters when returning from member detail', async ({page}) => { |
Contributor
There was a problem hiding this comment.
Rename test to match required title format
Please rename this test to the mandated 'what is tested - expected outcome' format (lowercase), e.g. returning from member detail - preserves applied filters.
As per coding guidelines, "Test names should follow format: 'what is tested - expected outcome' in lowercase".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/admin/members/list.test.ts` at line 55, Rename the test title
string in the test declaration test('preserves filters when returning from
member detail', async ({page}) => { ... }) to follow the required lowercase
"what is tested - expected outcome" format; change it to test('returning from
member detail - preserves applied filters', async ({page}) => { ... }) so the
test name matches guidelines.
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24504119691 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
rob-ghost
force-pushed
the
ber-3361-remove-feature-flag-release
branch
from
8346bea to
e9c550f
Compare
rob-ghost
added
preview
rob-ghost
force-pushed
the
ber-3361-remove-feature-flag-release
branch
from
e9c550f to
4ea9ad7
Compare
rob-ghost
added
preview
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24509461754 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24513932942 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
rob-ghost
force-pushed
the
ber-3361-remove-feature-flag-release
branch
from
da066ac to
6b3b0a0
Compare
Contributor
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 24515892736 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
ref https://linear.app/tryghost/issue/BER-3361 Adds E2E test for direct navigation to /members/import and prevents Ember's dynamic member route from capturing /members/import URLs.
rob-ghost
force-pushed
the
ber-3361-remove-feature-flag-release
branch
from
6b3b0a0 to
604331b
Compare
|
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
3 participants
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
This PR removes the
membersForwardrollout machinery and the/members-forwardroute so the released React members experience owns/membersand/members/importdirectly.What changed
membersForwardlabs flag and related feature-service/settings exposure/members-forwardroute so React owns the released members routes directlybackpathWhy
BER-3506 landed the route handoff foundation. This branch finishes the release by deleting the rollout gate and flag-specific behavior without mixing in the broader Ember dead-code cleanup.
Testing
YARN_IGNORE_ENGINES=1 yarn workspace @tryghost/admin test:unit src/members-route.test.tsxYARN_IGNORE_ENGINES=1 yarn workspace @tryghost/posts exec vitest run test/unit/views/members/members-actions.test.tsxcd ghost/core && YARN_IGNORE_ENGINES=1 yarn test:single test/unit/shared/labs.test.jsYARN_IGNORE_ENGINES=1 yarn workspace @tryghost/e2e test:types