Conversation
|
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:
WalkthroughConsolidates members routing and UI from Possibly related PRs
Suggested labels
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 |
jonatansberg
changed the title
jonatansberg
force-pushed
the
ber-3506-rework-feature-flagging-for-release
branch
from
ab27ec9 to
bcdc536
Compare
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23843231415 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
jonatansberg
force-pushed
the
ber-3506-rework-feature-flagging-for-release
branch
from
ee6d99d to
dbcc942
Compare
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23847254654 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
jonatansberg
changed the title
jonatansberg
force-pushed
the
ber-3506-rework-feature-flagging-for-release
branch
from
9cd2c2e to
eadca30
Compare
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23848656698 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23850456209 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
![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: 1c37481fb8
ℹ️ 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".
apps/admin/src/routes.tsx
Outdated
| path: "/members-forward", | ||
| // TODO: Remove once the legacy Ember members list is deleted. | ||
| handle: emberFallbackHandle, | ||
| loader: () => redirect("/members") |
There was a problem hiding this comment.
The compatibility route for /members-forward now uses redirect('/members'), which drops the existing query string. Any bookmarked/shared URL like /members-forward?filter=label:VIP or /members-forward?search=alice lands on an unfiltered members list, so old deep links no longer reproduce the same state after this migration. Build the redirect target from the incoming request URL so search is carried over.
Useful? React with 👍 / 👎.
| this.searchInput = page.locator([ | ||
| 'input[aria-label="Search members"]:visible', | ||
| 'input[aria-label="Search members mobile"]:visible' | ||
| ].join(', ')).first(); |
There was a problem hiding this comment.
This introduces a CSS selector-based locator (page.locator('input[...]')) for the members search field, which violates the explicit test rule in e2e/AGENTS.md (“Never use CSS/XPath selectors”) and is more brittle than role/label semantics. As the DOM structure or visibility behavior changes, this can silently target the wrong input and create flaky E2E behavior; prefer semantic Playwright locators here.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/posts/test/unit/views/members/import-members/modal.test.tsx (1)
56-74:⚠️ Potential issue | 🟠 MajorRestore the
URLoverrides inafterEach.
Object.defineProperty()mutates the globalURLobject directly, andvi.unstubAllGlobals()only restores globals modified byvi.stubGlobal(), not properties mutated withObject.defineProperty(). These changes will leak into subsequent tests and create order-dependent behavior.🔧 Suggested adjustment
describe('ImportMembersModal', () => { + let createObjectURLDescriptor: PropertyDescriptor | undefined; + let revokeObjectURLDescriptor: PropertyDescriptor | undefined; + beforeEach(() => { vi.stubGlobal('FileReader', MockFileReader); vi.stubGlobal('fetch', vi.fn(async () => new Response(null, {status: 202}))); + createObjectURLDescriptor = Object.getOwnPropertyDescriptor(URL, 'createObjectURL'); + revokeObjectURLDescriptor = Object.getOwnPropertyDescriptor(URL, 'revokeObjectURL'); Object.defineProperty(URL, 'createObjectURL', { configurable: true, writable: true, value: vi.fn(() => 'blob:mock/0') }); @@ afterEach(() => { + if (createObjectURLDescriptor) { + Object.defineProperty(URL, 'createObjectURL', createObjectURLDescriptor); + } else { + Reflect.deleteProperty(URL, 'createObjectURL'); + } + + if (revokeObjectURLDescriptor) { + Object.defineProperty(URL, 'revokeObjectURL', revokeObjectURLDescriptor); + } else { + Reflect.deleteProperty(URL, 'revokeObjectURL'); + } + vi.restoreAllMocks(); vi.unstubAllGlobals(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/test/unit/views/members/import-members/modal.test.tsx` around lines 56 - 74, The test overrides URL.createObjectURL and URL.revokeObjectURL via Object.defineProperty in beforeEach which mutates the global URL and is not reverted by vi.unstubAllGlobals; update the test to save the original descriptors (e.g., const originalCreate = Object.getOwnPropertyDescriptor(URL, 'createObjectURL') and originalRevoke similarly) before overriding, and in afterEach restore them using Object.defineProperty(URL, 'createObjectURL', originalCreate) and Object.defineProperty(URL, 'revokeObjectURL', originalRevoke) (or delete the properties to revert to defaults) so URL.createObjectURL and URL.revokeObjectURL are restored for other tests.
🧹 Nitpick comments (1)
e2e/tests/admin/members/search-and-filter.test.ts (1)
47-47: Reduce hardcoded/encoded filter URLs in tests.These
page.gotostrings work, but the manual encoding at Line 68 is hard to maintain. A tiny helper keeps intent readable and avoids encoding mistakes.Refactor sketch
+const gotoMembersWithFilter = async (page: import('@playwright/test').Page, filter: string) => { + const params = new URLSearchParams({filter}); + await page.goto(`/ghost/#/members?${params.toString()}`); +}; - await page.goto('/ghost/#/members?filter=label:VIP'); + await gotoMembersWithFilter(page, 'label:VIP'); - await page.goto('/ghost/#/members?filter=name:~%27Alice%27%2Blabel:Premium'); + await gotoMembersWithFilter(page, `name:~'Alice'+label:Premium`); - await page.goto('/ghost/#/members?filter=label:VIP%2Blabel:Premium'); + await gotoMembersWithFilter(page, 'label:VIP+label:Premium');Also applies to: 68-68, 88-88, 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/members/search-and-filter.test.ts` at line 47, Tests currently use hardcoded, manually-encoded filter query strings in page.goto (e.g., await page.goto('/ghost/#/members?filter=label:VIP')) which is brittle; add a small helper (e.g., encodeMembersFilter or buildMembersUrl) used by the test file search-and-filter.test.ts to produce the members URL with a properly encoded filter param, then replace the direct page.goto string usages (the occurrences around the current page.goto calls) with await page.goto(buildMembersUrl('label:VIP')) or similar; ensure the helper takes the raw filter string or an object and returns '/ghost/#/members?filter=' + encodeURIComponent(filter) so all instances (lines cited: the page.goto calls at the noted locations) consistently use it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin/src/members-route-gate.tsx`:
- Around line 8-12: The members gate currently blocks the React import route
because it unconditionally returns EmberFallback when membersForwardEnabled is
false; update the gating logic in members-route-gate.tsx to allow the import
path to bypass the flag by checking the current route (useLocation or useMatch)
and only render EmberFallback when membersForwardEnabled is false AND the
current route is NOT the import route (e.g., path '/members/import'); keep
rendering Outlet for the import route even when membersForwardEnabled is false
so EmberFallback only affects the main members flow.
In `@e2e/tests/admin/members-legacy/export.test.ts`:
- Around line 7-10: Suite inherits global labs state so it may run under the
forward/react members path; pin this legacy suite to the legacy lab by setting
the labs flag for the describe block (e.g., call test.use with
labs.membersForward = false) near the top of this file (around
usePerTestIsolation() / test.describe('Ghost Admin - Member Export', ...)) so
the suite always runs the legacy path regardless of default config.
In `@e2e/tests/admin/members-legacy/import.test.ts`:
- Line 10: The test binds MembersPage to the import route (MembersPage with
{route: 'members/import'}) which causes membersPage.goto() later to reopen the
import page instead of the members list; fix this by using two distinct page
objects or by creating the MembersPage for the list view separately: keep one
instance (e.g., importPage or importMembersPage) for the import modal/actions
and create a second MembersPage instance (or re-initialize membersPage)
targeting the list route (e.g., default 'members' or {route: 'members'}) prior
to the final membersPage.goto() and assertions so the post-import assertions run
against the actual list page.
---
Outside diff comments:
In `@apps/posts/test/unit/views/members/import-members/modal.test.tsx`:
- Around line 56-74: The test overrides URL.createObjectURL and
URL.revokeObjectURL via Object.defineProperty in beforeEach which mutates the
global URL and is not reverted by vi.unstubAllGlobals; update the test to save
the original descriptors (e.g., const originalCreate =
Object.getOwnPropertyDescriptor(URL, 'createObjectURL') and originalRevoke
similarly) before overriding, and in afterEach restore them using
Object.defineProperty(URL, 'createObjectURL', originalCreate) and
Object.defineProperty(URL, 'revokeObjectURL', originalRevoke) (or delete the
properties to revert to defaults) so URL.createObjectURL and URL.revokeObjectURL
are restored for other tests.
---
Nitpick comments:
In `@e2e/tests/admin/members/search-and-filter.test.ts`:
- Line 47: Tests currently use hardcoded, manually-encoded filter query strings
in page.goto (e.g., await page.goto('/ghost/#/members?filter=label:VIP')) which
is brittle; add a small helper (e.g., encodeMembersFilter or buildMembersUrl)
used by the test file search-and-filter.test.ts to produce the members URL with
a properly encoded filter param, then replace the direct page.goto string usages
(the occurrences around the current page.goto calls) with await
page.goto(buildMembersUrl('label:VIP')) or similar; ensure the helper takes the
raw filter string or an object and returns '/ghost/#/members?filter=' +
encodeURIComponent(filter) so all instances (lines cited: the page.goto calls at
the noted locations) consistently use it.
🪄 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: f0052e1e-3603-4830-b5a4-106f31d278e0
📒 Files selected for processing (31)
apps/admin/src/layout/app-sidebar/member-sidebar-views.tsapps/admin/src/layout/app-sidebar/nav-content.helpers.test.tsapps/admin/src/layout/app-sidebar/nav-content.helpers.tsapps/admin/src/layout/app-sidebar/nav-content.tsxapps/admin/src/members-route-gate.tsxapps/admin/src/routes.tsxapps/posts/src/routes.tsxapps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/import-members/state.tsapps/posts/src/views/members/components/bulk-action-modals/import-members/upload.tsapps/posts/src/views/members/components/members-actions.tsxapps/posts/test/unit/views/members/import-members/modal.test.tsxapps/posts/test/unit/views/members/import-members/upload.test.tsapps/posts/test/unit/views/members/members-actions.test.tsxe2e/helpers/pages/admin/members/index.tse2e/helpers/pages/admin/members/members-list-page.tse2e/tests/admin/members-forward/export.test.tse2e/tests/admin/members-legacy/disable-commenting.test.tse2e/tests/admin/members-legacy/export.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/saved-views.test.tse2e/tests/admin/members/search-and-filter.test.tse2e/tests/admin/members/virtual-window.test.ts
💤 Files with no reviewable changes (2)
- apps/posts/src/routes.tsx
- e2e/tests/admin/members-forward/export.test.ts
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23852169224 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/tests/admin/members-legacy/disable-commenting.test.ts (1)
174-186:⚠️ Potential issue | 🟡 MinorMissing
test.use({labs: {membersForward: false}})in second describe block.The
'Ghost Admin - Disable Commenting Cache Invalidation'suite usesMembersPageto navigate to/members, but unlike the first describe block, it lacks thelabs: {membersForward: false}override. This may cause the test to run against the React UI instead of the intended Ember legacy UI.🔧 Proposed fix
test.describe('Ghost Admin - Disable Commenting Cache Invalidation', () => { + test.use({labs: {membersForward: false}}); + let memberFactory: MemberFactory; let postFactory: PostFactory; let commentFactory: CommentFactory;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/members-legacy/disable-commenting.test.ts` around lines 174 - 186, The second test suite "Ghost Admin - Disable Commenting Cache Invalidation" is missing the Playwright override to force the legacy Ember members UI; add test.use({ labs: { membersForward: false } }) at the top of the describe block for "Ghost Admin - Disable Commenting Cache Invalidation" so that MembersPage navigation to /members (and any usage of MembersPage, createMemberFactory/createPostFactory/createCommentFactory) runs against the legacy UI instead of the React UI.
🧹 Nitpick comments (1)
apps/admin/src/members-route-gate.tsx (1)
5-15: Consider normalizing pathname for trailing slash edge case.The exact equality check
location.pathname === "/members"won't match URLs with a trailing slash (/members/). React Router v6+ does not automatically normalize trailing slashes, so this is a valid edge case worth defending against.♻️ Optional fix for trailing slash handling
export function MembersRouteGate() { const membersForwardEnabled = useFeatureFlag("membersForward"); const location = useLocation(); - const isMembersListRoute = location.pathname === "/members"; + const isMembersListRoute = location.pathname === "/members" || location.pathname === "/members/"; if (!membersForwardEnabled && isMembersListRoute) { return <EmberFallback />; } return <Outlet />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/members-route-gate.tsx` around lines 5 - 15, The equality check in MembersRouteGate misses paths with a trailing slash (e.g. "/members/"); update the check by normalizing location.pathname before comparing: compute a normalizedPath (e.g. strip trailing slashes with something like location.pathname.replace(/\/+$/, '') || '/') and then set isMembersListRoute = normalizedPath === "/members"; keep the rest of the logic (return <EmberFallback /> when !membersForwardEnabled && isMembersListRoute, otherwise <Outlet />) so EmberFallback triggers for both "/members" and "/members/".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@e2e/tests/admin/members-legacy/disable-commenting.test.ts`:
- Around line 174-186: The second test suite "Ghost Admin - Disable Commenting
Cache Invalidation" is missing the Playwright override to force the legacy Ember
members UI; add test.use({ labs: { membersForward: false } }) at the top of the
describe block for "Ghost Admin - Disable Commenting Cache Invalidation" so that
MembersPage navigation to /members (and any usage of MembersPage,
createMemberFactory/createPostFactory/createCommentFactory) runs against the
legacy UI instead of the React UI.
---
Nitpick comments:
In `@apps/admin/src/members-route-gate.tsx`:
- Around line 5-15: The equality check in MembersRouteGate misses paths with a
trailing slash (e.g. "/members/"); update the check by normalizing
location.pathname before comparing: compute a normalizedPath (e.g. strip
trailing slashes with something like location.pathname.replace(/\/+$/, '') ||
'/') and then set isMembersListRoute = normalizedPath === "/members"; keep the
rest of the logic (return <EmberFallback /> when !membersForwardEnabled &&
isMembersListRoute, otherwise <Outlet />) so EmberFallback triggers for both
"/members" and "/members/".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 111bb797-ba08-4404-a23c-d96264f21a85
📒 Files selected for processing (14)
apps/admin/src/members-route-gate.tsxapps/admin/src/routes.tsxapps/posts/src/views/members/components/members-header-search.tsxapps/posts/test/unit/views/members/import-members/modal.test.tsxe2e/helpers/pages/admin/members/members-list-page.tse2e/tests/admin/members-legacy/disable-commenting.test.tse2e/tests/admin/members-legacy/export.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/list.test.ts
✅ Files skipped from review due to trivial changes (5)
- e2e/tests/admin/members-legacy/filter-actions.test.ts
- e2e/tests/admin/members-legacy/import.test.ts
- e2e/tests/admin/members-legacy/export.test.ts
- apps/admin/src/routes.tsx
- apps/posts/test/unit/views/members/import-members/modal.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/helpers/pages/admin/members/members-list-page.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed8639a2d7
ℹ️ 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".
| return; | ||
| } | ||
|
|
||
| navigate('/members'); |
There was a problem hiding this comment.
Closing the import route unconditionally navigates to /members, which drops the current filter/search query when a user opens import from a scoped list such as /members?filter=label:VIP and then cancels/closes. This is a regression from the previous in-place modal behavior because users lose their current member view context after closing; the close target should be derived from the prior location/query (or use back/replace semantics) unless a labeled-import redirect is being applied.
Useful? React with 👍 / 👎.
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23853671589 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
jonatansberg
force-pushed
the
ber-3506-rework-feature-flagging-for-release
branch
from
ed8639a to
37c6085
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/posts/test/unit/views/members/members-actions.test.tsx (1)
47-54: Consider clearing mock call history inbeforeEach.The
mockUseNavigateandmockUseLocationmocks retain call history across tests. WhilemockReturnValueis reset, consider addingvi.clearAllMocks()to ensure test isolation.♻️ Suggested improvement
beforeEach(() => { importModalPropsRef.current = null; + vi.clearAllMocks(); mockUseLocation.mockReturnValue({ pathname: '/members', search: '' }); mockUseNavigate.mockReturnValue(vi.fn()); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/test/unit/views/members/members-actions.test.tsx` around lines 47 - 54, The tests keep mock call history between runs which can leak state; in the beforeEach that sets importModalPropsRef.current, mockUseLocation, and mockUseNavigate, call vi.clearAllMocks() (or vi.resetAllMocks()) at the start of the beforeEach to clear mock call history for mockUseNavigate and mockUseLocation so each test is isolated; update the beforeEach surrounding mockUseNavigate/mockUseLocation to include this clear/reset call before reassigning mockReturnValue.e2e/tests/admin/members/virtual-window.test.ts (1)
83-104: Direct locator usage is acceptable but could be encapsulated.The use of
page.getByTestId('members-list').getByTestId('members-list-item')directly in the test bypasses the page object'smemberRowslocator. While this works and uses alloweddata-testidselectors, consider whether this specific locator pattern should be exposed through theMembersPagepage object for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/members/virtual-window.test.ts` around lines 83 - 104, The test directly uses page.getByTestId('members-list').getByTestId('members-list-item') (stored as reactMemberRows) instead of the MembersPage page object locator; update the test to use the existing membersPage.memberRows locator (or add a memberRows getter to MembersPage if it doesn't exist) and then use that locator for counting, nth(), and attribute/text reads so the test consistently uses the MembersPage API alongside membersPage.scrollUntilMaxRenderedIndexAtLeast and membersPage.getScrollParentScrollTop; ensure the subsequent targetRowLocator, targetRow.index/text/scrollTop extraction, click and assertions remain identical but reference the page object locator.e2e/tests/admin/members-legacy/export.test.ts (1)
14-22: Consider moving helper function to outer scope.The
extractDownloadedContentSpecificsfunction is defined inside the test describe block but doesn't depend on any test-scoped variables. Moving it to the module scope would improve readability and satisfy the static analysis hint.♻️ Suggested refactor
import {MemberFactory, createMemberFactory} from '@/data-factory'; import {MembersPage} from '@/helpers/pages'; +function extractDownloadedContentSpecifics(content: string) { + const contentIds = content.match(/[a-z0-9]{24}/gm); + const contentTimestamps = content.match(/\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z/gm); + + return { + contentIds, + contentTimestamps + }; +} + usePerTestIsolation(); test.describe('Ghost Admin - Member Export', () => { test.use({labs: {membersForward: false}}); let memberFactory: MemberFactory; - - function extractDownloadedContentSpecifics(content: string) { - const contentIds = content.match(/[a-z0-9]{24}/gm); - const contentTimestamps = content.match(/\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z/gm); - - return { - contentIds, - contentTimestamps - }; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/members-legacy/export.test.ts` around lines 14 - 22, The helper function extractDownloadedContentSpecifics is currently declared inside the test describe block but it does not use any test-scoped variables; move its declaration to module (outer) scope so it lives alongside other top-level helpers, keeping its signature and return shape unchanged and updating any internal references in the tests to use the same function name; ensure imports/exports are adjusted only if you need the helper in other files.apps/admin/src/members-route-gate.test.tsx (1)
24-44: Add test coverage for the flag-enabled scenario.The current tests only cover behavior when
membersForwardis disabled. Add tests to verify behavior when the flag is enabled:
/members/(or/members) with flag enabled should render the ReactOutlet- Optionally verify that
useFeatureFlagis called with'membersForward'The existing tests for disabled behavior (both
/members/and/members/importpaths) are adequate, though you could add an explicit test for the/memberspath (no trailing slash) for clarity, given thebeforeEachsetup.📝 Suggested test case
+ it('renders React Outlet for /members/ when the flag is enabled', () => { + mockUseFeatureFlag.mockReturnValue(true); + mockUseLocation.mockReturnValue({pathname: '/members/'}); + + render(<MembersRouteGate />); + + expect(screen.getByTestId('outlet')).toBeInTheDocument(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/members-route-gate.test.tsx` around lines 24 - 44, Add tests verifying behavior when the feature flag is enabled: in the MembersRouteGate test suite, create one or two tests that mockUseFeatureFlag to return true and mockUseLocation to return {pathname: '/members'} (and optionally '/members/' ) then render <MembersRouteGate /> and assert the React Outlet is rendered (expect screen.getByTestId('outlet') toBeInTheDocument()); also add an assertion that useFeatureFlag was called with 'membersForward' (or verify mockUseFeatureFlag was invoked with that key) to ensure the flag is actually checked.
🤖 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/members-route-gate.test.tsx`:
- Around line 24-44: Add tests verifying behavior when the feature flag is
enabled: in the MembersRouteGate test suite, create one or two tests that
mockUseFeatureFlag to return true and mockUseLocation to return {pathname:
'/members'} (and optionally '/members/' ) then render <MembersRouteGate /> and
assert the React Outlet is rendered (expect screen.getByTestId('outlet')
toBeInTheDocument()); also add an assertion that useFeatureFlag was called with
'membersForward' (or verify mockUseFeatureFlag was invoked with that key) to
ensure the flag is actually checked.
In `@apps/posts/test/unit/views/members/members-actions.test.tsx`:
- Around line 47-54: The tests keep mock call history between runs which can
leak state; in the beforeEach that sets importModalPropsRef.current,
mockUseLocation, and mockUseNavigate, call vi.clearAllMocks() (or
vi.resetAllMocks()) at the start of the beforeEach to clear mock call history
for mockUseNavigate and mockUseLocation so each test is isolated; update the
beforeEach surrounding mockUseNavigate/mockUseLocation to include this
clear/reset call before reassigning mockReturnValue.
In `@e2e/tests/admin/members-legacy/export.test.ts`:
- Around line 14-22: The helper function extractDownloadedContentSpecifics is
currently declared inside the test describe block but it does not use any
test-scoped variables; move its declaration to module (outer) scope so it lives
alongside other top-level helpers, keeping its signature and return shape
unchanged and updating any internal references in the tests to use the same
function name; ensure imports/exports are adjusted only if you need the helper
in other files.
In `@e2e/tests/admin/members/virtual-window.test.ts`:
- Around line 83-104: The test directly uses
page.getByTestId('members-list').getByTestId('members-list-item') (stored as
reactMemberRows) instead of the MembersPage page object locator; update the test
to use the existing membersPage.memberRows locator (or add a memberRows getter
to MembersPage if it doesn't exist) and then use that locator for counting,
nth(), and attribute/text reads so the test consistently uses the MembersPage
API alongside membersPage.scrollUntilMaxRenderedIndexAtLeast and
membersPage.getScrollParentScrollTop; ensure the subsequent targetRowLocator,
targetRow.index/text/scrollTop extraction, click and assertions remain identical
but reference the page object locator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac758a1e-2777-4acf-b951-458791382ced
📒 Files selected for processing (35)
apps/admin/src/layout/app-sidebar/member-sidebar-views.tsapps/admin/src/layout/app-sidebar/nav-content.helpers.test.tsapps/admin/src/layout/app-sidebar/nav-content.helpers.tsapps/admin/src/layout/app-sidebar/nav-content.tsxapps/admin/src/members-route-gate.test.tsxapps/admin/src/members-route-gate.tsxapps/admin/src/routes.tsxapps/posts/src/components/virtual-table/virtual-list-window.test.tsapps/posts/src/routes.tsxapps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/import-members/state.tsapps/posts/src/views/members/components/bulk-action-modals/import-members/upload.tsapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/components/members-header-search.tsxapps/posts/test/unit/views/members/import-members/modal.test.tsxapps/posts/test/unit/views/members/import-members/upload.test.tsapps/posts/test/unit/views/members/members-actions.test.tsxe2e/helpers/pages/admin/members/index.tse2e/helpers/pages/admin/members/members-list-page.tse2e/tests/admin/members-forward/export.test.tse2e/tests/admin/members-legacy/disable-commenting.test.tse2e/tests/admin/members-legacy/export.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/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.ts
💤 Files with no reviewable changes (2)
- apps/posts/src/routes.tsx
- e2e/tests/admin/members-forward/export.test.ts
✅ Files skipped from review due to trivial changes (13)
- e2e/tests/admin/members-legacy/member-activity-events.test.ts
- e2e/tests/admin/members-legacy/filter-actions.test.ts
- e2e/tests/admin/members-legacy/disable-commenting.test.ts
- apps/posts/src/views/members/components/members-header-search.tsx
- e2e/tests/admin/members-legacy/members.test.ts
- e2e/helpers/pages/admin/members/index.ts
- e2e/tests/admin/members-legacy/stripe-webhooks.test.ts
- apps/posts/test/unit/views/members/import-members/modal.test.tsx
- e2e/tests/admin/members/search-and-filter.test.ts
- apps/posts/test/unit/views/members/import-members/upload.test.ts
- apps/posts/src/views/members/components/bulk-action-modals/import-members/state.ts
- apps/admin/src/members-route-gate.tsx
- e2e/tests/admin/members-legacy/impersonation.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- e2e/tests/admin/members/list.test.ts
- e2e/tests/admin/members-legacy/import.test.ts
- apps/admin/src/layout/app-sidebar/nav-content.helpers.ts
- e2e/tests/admin/members/saved-views.test.ts
- apps/posts/src/views/members/components/bulk-action-modals/import-members/upload.ts
- e2e/tests/admin/members/bulk-actions.test.ts
- apps/admin/src/layout/app-sidebar/nav-content.helpers.test.ts
- apps/admin/src/layout/app-sidebar/nav-content.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37c6085f1c
ℹ️ 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".
| return; | ||
| } | ||
|
|
||
| navigate(`/members${currentSearch}`); |
There was a problem hiding this comment.
Closing the route-backed import modal currently does a normal navigate('/members...'), which pushes a new history entry. In the common flow (members list → import modal → close), pressing Back reopens the modal instead of returning to the previous page, which is a UX regression from the old in-place modal behavior. This occurs whenever users close/cancel import without a label redirect; use replace/back semantics so dismissing the modal unwinds history instead of stacking another entry.
Useful? React with 👍 / 👎.
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23897853783 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release This keeps the members list and import flow on the stable /members routes, preserves the Ember fallback behind the flag, and aligns the e2e suite layout around members and members-legacy.
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release This finishes the members and members-legacy e2e split by moving stale React tests off members-forward and aligning detail navigation with the current members list UI.
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release Kept route-specific members coverage and removed duplicate list, export, and import pass-through tests
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release Removed the dead members route helper and inlined the fixed legacy active-route list in the sidebar
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release Reverted the over-reduced React members e2e changes and kept only the route and page-object adaptations needed for this branch
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release The members page object was matching Ember's hidden search input from EmberRoot instead of the visible React control.
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release Preserve members-forward query state, keep import on the React path, pin legacy member e2e coverage to Ember, and scope the React members page object away from hidden Ember rows.
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release This restores the members import return context, fixes the React virtual-window test on the shared route, and addresses the remaining review comments pulled back in during the rebase.
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release This scopes the React members page object away from hidden Ember controls and updates the multiselect filter suite to the current helper names after rebasing onto main.
jonatansberg
force-pushed
the
ber-3506-rework-feature-flagging-for-release
branch
from
37c6085 to
583b041
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/posts/src/views/members/components/members-actions.tsx (1)
195-196: Intentional no-op is appropriate for route-driven modal.Since modal open/close is controlled by navigation, the no-op
handleImportModalOpenChangecorrectly decouples the modal's internal state changes from triggering side effects. Consider adding a brief comment for future maintainers.📝 Suggested clarification comment
- const handleImportModalOpenChange = useCallback(() => {}, []); + // No-op: modal visibility is route-driven via `/members/import` + const handleImportModalOpenChange = useCallback(() => {}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/posts/src/views/members/components/members-actions.tsx` around lines 195 - 196, The empty function handleImportModalOpenChange is an intentional no-op because the modal's open/close state is driven by routing; add a short clarifying comment immediately above the handleImportModalOpenChange declaration explaining that it is intentionally empty to decouple modal internal events from navigation-driven state and to prevent future maintainers from removing or modifying it.
🤖 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/posts/src/views/members/components/members-actions.tsx`:
- Around line 195-196: The empty function handleImportModalOpenChange is an
intentional no-op because the modal's open/close state is driven by routing; add
a short clarifying comment immediately above the handleImportModalOpenChange
declaration explaining that it is intentionally empty to decouple modal internal
events from navigation-driven state and to prevent future maintainers from
removing or modifying it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae5f1fb9-04a6-4a8b-bdce-6f350d15146c
📒 Files selected for processing (36)
apps/admin/src/layout/app-sidebar/member-sidebar-views.tsapps/admin/src/layout/app-sidebar/nav-content.helpers.test.tsapps/admin/src/layout/app-sidebar/nav-content.helpers.tsapps/admin/src/layout/app-sidebar/nav-content.tsxapps/admin/src/members-route-gate.test.tsxapps/admin/src/members-route-gate.tsxapps/admin/src/routes.tsxapps/posts/src/components/virtual-table/virtual-list-window.test.tsapps/posts/src/routes.tsxapps/posts/src/views/members/components/bulk-action-modals/import-members-modal.tsxapps/posts/src/views/members/components/bulk-action-modals/import-members/state.tsapps/posts/src/views/members/components/bulk-action-modals/import-members/upload.tsapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/components/members-header-search.tsxapps/posts/test/unit/views/members/import-members/modal.test.tsxapps/posts/test/unit/views/members/import-members/upload.test.tsapps/posts/test/unit/views/members/members-actions.test.tsxe2e/helpers/pages/admin/members/index.tse2e/helpers/pages/admin/members/members-list-page.tse2e/tests/admin/members-forward/export.test.tse2e/tests/admin/members-legacy/disable-commenting.test.tse2e/tests/admin/members-legacy/export.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.ts
💤 Files with no reviewable changes (2)
- apps/posts/src/routes.tsx
- e2e/tests/admin/members-forward/export.test.ts
✅ Files skipped from review due to trivial changes (12)
- apps/posts/src/views/members/components/members-header-search.tsx
- e2e/tests/admin/members-legacy/member-activity-events.test.ts
- e2e/tests/admin/members-legacy/disable-commenting.test.ts
- e2e/tests/admin/members-legacy/filter-actions.test.ts
- e2e/tests/admin/members-legacy/stripe-webhooks.test.ts
- e2e/tests/admin/members/search-and-filter.test.ts
- e2e/tests/admin/members-legacy/impersonation.test.ts
- e2e/tests/admin/members/saved-views.test.ts
- apps/admin/src/members-route-gate.test.tsx
- apps/posts/test/unit/views/members/members-actions.test.tsx
- e2e/tests/admin/members-legacy/members.test.ts
- apps/posts/src/components/virtual-table/virtual-list-window.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- e2e/helpers/pages/admin/members/index.ts
- apps/posts/test/unit/views/members/import-members/modal.test.tsx
- e2e/tests/admin/members/tier-filter-search.test.ts
- apps/admin/src/layout/app-sidebar/member-sidebar-views.ts
- apps/admin/src/layout/app-sidebar/nav-content.helpers.ts
- apps/admin/src/members-route-gate.tsx
- apps/posts/src/views/members/components/bulk-action-modals/import-members/upload.ts
- apps/posts/test/unit/views/members/import-members/upload.test.ts
- apps/admin/src/layout/app-sidebar/nav-content.helpers.test.ts
- e2e/tests/admin/members/virtual-window.test.ts
- e2e/tests/admin/members/export.test.ts
- e2e/tests/admin/members-legacy/export.test.ts
- e2e/tests/admin/members-legacy/import.test.ts
- e2e/helpers/pages/admin/members/members-list-page.ts
- e2e/tests/admin/members/bulk-actions.test.ts
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release The app now canonicalizes the legacy label filter into the encoded React route form, so the redirect test needs to assert the normalized URL instead of the raw legacy query string.
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23899244219 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f01eb5f9c3
ℹ️ 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".
| await membersPage.getMenuItem('Import members').click(); | ||
|
|
||
| const importDialog = page.getByRole('dialog', {name: 'Import members'}); | ||
| const fileInput = importDialog.locator('input[type="file"]').first(); |
There was a problem hiding this comment.
This test reintroduces a raw CSS locator (input[type="file"]), which violates the E2E rule in e2e/AGENTS.md (“Never use CSS/XPath selectors”) and is brittle when multiple file inputs are present (for example hidden dropzone inputs plus active dialog inputs), making the import flow test prone to targeting the wrong element. Please switch this to a semantic/test-id based locator in the dialog scope so the test remains stable as markup evolves.
Useful? React with 👍 / 👎.
| const location = useLocation(); | ||
| const sharedViews = useSharedViews('members'); | ||
| const isOnMembersForward = location.pathname === '/members-forward'; | ||
| const isOnMembersListRoute = location.pathname === '/members'; |
There was a problem hiding this comment.
The members route gate now explicitly treats /members/ as the members list, but this check only matches '/members', so a trailing-slash URL leaves all member saved views inactive even when the filter matches. This creates inconsistent sidebar state for bookmarked/manual /members/ URLs; normalizing trailing slashes here (as done in the gate) avoids that mismatch.
Useful? React with 👍 / 👎.
ref https://linear.app/ghost/issue/BER-3506/rework-feature-flagging-for-release Gate /members/import with the same membersForward split as the rest of the members surface, and have Ember opt out to react-fallback when React owns that route so only one import modal can mount.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9697d4df66
ℹ️ 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".
| const postsExpanded = savedPostsExpanded; | ||
| const isOnMembersForward = location.pathname === '/members-forward'; | ||
| const hasActiveMemberView = isOnMembersForward && memberViews.some(view => view.isActive); | ||
| const isOnMembersRoute = location.pathname === '/members' || location.pathname === '/members/import'; |
There was a problem hiding this comment.
The new check only treats '/members' and '/members/import' as React-owned members routes, so a valid trailing-slash URL like /members/ is treated as off-route and isMembersNavActive falls back to Ember route state, leaving the Members nav item inactive. This is a regression for bookmarked/manual URLs (and members-route-gate.test.tsx now explicitly treats /members/ as a members route), so the pathname should be normalized (for example by trimming a trailing slash) before this comparison.
Useful? React with 👍 / 👎.
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 23902490780 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
1 participant
Summary
membersForwardso it gates React rendering on/membersinstead of changing sidebar URLs/members/importthrough the React members UI while keeping Ember fallback routes in placemembers/and Ember coverage lives undermembers-legacy/