Redirect /ghost/#/my-profile to current staff profile#26588
Redirect /ghost/#/my-profile to current staff profile#26588kevinansfield merged 6 commits intoTryGhost:mainfrom
Conversation
ref n/a Added settings app route normalization and tests, including an e2e deep-link check.
|
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:
WalkthroughAdds a MyProfileRedirect React component that reads the current user and redirects /my-profile to /settings/staff/{currentUser.slug}. Registers a "my-profile" route using that component. Introduces an AdminStaffDetailsPage page object with locators and gotoMyProfile, re-exports it from admin settings helpers, and adds a Playwright e2e test that navigates to /ghost/#/my-profile, inspects the user-detail-modal slug, and asserts the URL ends with /ghost/#/settings/staff/{slug}. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
kevinansfield
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/admin/settings/staff.test.ts`:
- Around line 15-17: Replace the dynamic RegExp URL assertion to avoid RegExp
construction: instead of using new RegExp(`/ghost/#/settings/staff/${slug}$`),
assert against the resolved page URL (e.g., use page.url() and
expect(...).toContain(...) or a pre-escaped string) referencing the existing
slug variable and page object so the line becomes a simple substring check
(expect(page.url()).toContain(`/ghost/#/settings/staff/${slug}`)) rather than
constructing a RegExp for toHaveURL.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/admin-x-settings/src/components/providers/settings-router.tsxapps/admin-x-settings/src/components/settings/general/user-detail-modal.tsxapps/admin-x-settings/test/acceptance/permissions.test.tse2e/tests/admin/settings/staff.test.ts
ref n/a Sorted imports and updated the e2e assertion style to satisfy package lint rules.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
e2e/tests/admin/settings/staff.test.ts (1)
12-13: LGTM — dynamicRegExpconstruction correctly replaced with static literals.Both URL and input-value assertions now use static regex literals, eliminating the prior ReDoS risk flagged in the previous review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/settings/staff.test.ts` around lines 12 - 13, Replace any dynamically constructed RegExp usage with static regex literals in the staff settings assertions: update the slugInput assertion (used with toHaveValue) and the page URL assertion (used with toHaveURL) to use static regex literals like /^(?!me$).+/ and /\/ghost\/#\/settings\/staff\/(?!me$)[^/]+$/ respectively to avoid ReDoS risks; ensure the assertions reference the existing slugInput variable and page object and keep the negative lookahead patterns intact.
🧹 Nitpick comments (1)
e2e/tests/admin/settings/staff.test.ts (1)
1-14: Consider introducing a Page Object per ADR-0002.All locator and navigation logic is currently inlined. Per the ADR-0002 requirement to use the Page Object pattern, the
UserDetailModalinteractions and the staff page navigation could be encapsulated in a dedicated Page Object (e.g.,StaffPage/UserDetailModalPage), making them reusable across future staff-settings tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/settings/staff.test.ts` around lines 1 - 14, Extract the inline navigation and locator logic from the test "redirects staff/me to the current user profile" into Page Object(s) per ADR-0002: create a StaffPage with a method to navigateToStaffMe (wraps page.goto('/ghost/#/settings/staff/me')) and a getter for the UserDetailModal, and create a UserDetailModalPage exposing slugInput and assertions (e.g., isVisible, getSlugValue) so the test uses StaffPage.navigateToStaffMe(), StaffPage.userDetailModal.getSlugValue() / isVisible() and StaffPage.assertUrlIsUserProfile() instead of directly calling page.goto, page.getByTestId('user-detail-modal'), or userDetailModal.getByRole('textbox', {name: 'Slug'}); update the test to import and use these Page Objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/tests/admin/settings/staff.test.ts`:
- Around line 12-13: Replace any dynamically constructed RegExp usage with
static regex literals in the staff settings assertions: update the slugInput
assertion (used with toHaveValue) and the page URL assertion (used with
toHaveURL) to use static regex literals like /^(?!me$).+/ and
/\/ghost\/#\/settings\/staff\/(?!me$)[^/]+$/ respectively to avoid ReDoS risks;
ensure the assertions reference the existing slugInput variable and page object
and keep the negative lookahead patterns intact.
---
Nitpick comments:
In `@e2e/tests/admin/settings/staff.test.ts`:
- Around line 1-14: Extract the inline navigation and locator logic from the
test "redirects staff/me to the current user profile" into Page Object(s) per
ADR-0002: create a StaffPage with a method to navigateToStaffMe (wraps
page.goto('/ghost/#/settings/staff/me')) and a getter for the UserDetailModal,
and create a UserDetailModalPage exposing slugInput and assertions (e.g.,
isVisible, getSlugValue) so the test uses StaffPage.navigateToStaffMe(),
StaffPage.userDetailModal.getSlugValue() / isVisible() and
StaffPage.assertUrlIsUserProfile() instead of directly calling page.goto,
page.getByTestId('user-detail-modal'), or userDetailModal.getByRole('textbox',
{name: 'Slug'}); update the test to import and use these Page Objects.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/admin-x-settings/src/components/providers/settings-router.tsxe2e/tests/admin/settings/staff.test.ts
| return; | ||
| } | ||
|
|
||
| const normalizedRoute = route.replace(/^staff\/me(?=\/|$)/, `staff/${currentUser.slug}`); |
There was a problem hiding this comment.
What if there's a staff member with the slug "me"?
|
|
||
| test.describe('Ghost Admin - Staff settings', () => { | ||
| test('redirects staff/me to the current user profile', async ({page}) => { | ||
| await page.goto('/ghost/#/settings/staff/me'); |
There was a problem hiding this comment.
We should create a page object in /helpers/pages/admin/settings/.
| test('redirects staff/me to the current user profile', async ({page}) => { | ||
| await page.goto('/ghost/#/settings/staff/me'); | ||
|
|
||
| const userDetailModal = page.getByTestId('user-detail-modal'); |
There was a problem hiding this comment.
Locators should be part of the page object.
| const userDetailModal = page.getByTestId('user-detail-modal'); | ||
| await expect(userDetailModal).toBeVisible(); | ||
|
|
||
| const slugInput = userDetailModal.getByRole('textbox', {name: 'Slug'}); |
There was a problem hiding this comment.
We can simplify this then to something like:
const StaffDetailsPage = new AdminStaffDetailsPage(page);
await StaffDetailsPage.goto('/me');
const slug = getSlug(page);
await expect(slug.value).toEqual('me');
ref n/a Adds a top-level my-profile route in the React admin shell and removes the temporary staff/me alias from admin-x-settings to avoid slug clashes.
kevinansfield
changed the title
ref n/a Adds a settings staff details page object and updates the my-profile redirect e2e test to use it per review feedback.
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 `@apps/admin/src/my-profile-redirect.tsx`:
- Around line 5-9: The component MyProfileRedirect currently renders null when
useCurrentUser returns undefined even if the hook errored; update the call to
useCurrentUser to also destructure isError (e.g., const { data: currentUser,
isError } = useCurrentUser()) and change the render guard so you only return
null while loading (i.e., when !currentUser && !isError); if isError is true
render a graceful fallback (redirect to a safe route or show an error UI) so the
route does not stay blank.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/admin/src/my-profile-redirect.tsxapps/admin/src/routes.tsxe2e/tests/admin/settings/staff.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/helpers/pages/admin/settings/staff-details-page.ts (1)
5-6: Add explicitpublicmodifier to locators.Both
userDetailModalandslugInputlack the explicitpublickeyword. While TypeScript's default visibility ispublic, the coding guidelines require locators to be explicitly declared aspublic readonlywhen used in assertions.♻️ Proposed fix
- readonly userDetailModal: Locator; - readonly slugInput: Locator; + public readonly userDetailModal: Locator; + public readonly slugInput: Locator;As per coding guidelines: "Page Objects should expose locators as
public readonlywhen 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/settings/staff-details-page.ts` around lines 5 - 6, The properties userDetailModal and slugInput are missing the explicit public modifier; update their declarations to be public readonly userDetailModal and public readonly slugInput so they follow the Page Object guideline that locators used in assertions must be exposed as public readonly (locate these symbols in the admin settings Staff Details page class and adjust their declarations accordingly).e2e/tests/admin/settings/staff.test.ts (1)
5-14: Consider adding explicit AAA section comments for clarity.The test is logically correct and follows the AAA structure, but the sections aren't visually separated. The coding guidelines require "clear sections" for the AAA pattern.
♻️ Proposed refactor
test('redirects my-profile to the current user profile', async ({page}) => { + // Arrange const staffDetailsPage = new AdminStaffDetailsPage(page); + // Act await staffDetailsPage.gotoMyProfile(); + // Assert await expect(staffDetailsPage.userDetailModal).toBeVisible(); await expect(staffDetailsPage.slugInput).toBeVisible(); await expect(staffDetailsPage.slugInput).toHaveValue(/^(?!me$).+/); await expect(page).toHaveURL(/\/ghost\/#\/settings\/staff\/(?!me$)[^/]+$/); });As per coding guidelines: "Use the AAA Pattern (Arrange, Act, Assert) in E2E tests with clear sections."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/settings/staff.test.ts` around lines 5 - 14, Add explicit AAA (Arrange, Act, Assert) section comments to the test 'redirects my-profile to the current user profile' to follow coding guidelines: mark the setup with "// Arrange" before constructing AdminStaffDetailsPage and any preconditions, mark the action with "// Act" immediately before calling staffDetailsPage.gotoMyProfile(), and mark assertions with "// Assert" before the expect(...) checks (references: test name, AdminStaffDetailsPage, gotoMyProfile, staffDetailsPage.userDetailModal, staffDetailsPage.slugInput, page URL assertion). Ensure comments are placed so each logical block is visually separated and in the same test function.
🤖 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/helpers/pages/admin/settings/staff-details-page.ts`:
- Around line 5-6: The properties userDetailModal and slugInput are missing the
explicit public modifier; update their declarations to be public readonly
userDetailModal and public readonly slugInput so they follow the Page Object
guideline that locators used in assertions must be exposed as public readonly
(locate these symbols in the admin settings Staff Details page class and adjust
their declarations accordingly).
In `@e2e/tests/admin/settings/staff.test.ts`:
- Around line 5-14: Add explicit AAA (Arrange, Act, Assert) section comments to
the test 'redirects my-profile to the current user profile' to follow coding
guidelines: mark the setup with "// Arrange" before constructing
AdminStaffDetailsPage and any preconditions, mark the action with "// Act"
immediately before calling staffDetailsPage.gotoMyProfile(), and mark assertions
with "// Assert" before the expect(...) checks (references: test name,
AdminStaffDetailsPage, gotoMyProfile, staffDetailsPage.userDetailModal,
staffDetailsPage.slugInput, page URL assertion). Ensure comments are placed so
each logical block is visually separated and in the same test function.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/helpers/pages/admin/settings/index.tse2e/helpers/pages/admin/settings/staff-details-page.tse2e/tests/admin/settings/staff.test.ts
ref n/a Redirects to a safe route when the current-user query errors or resolves without a user instead of rendering an empty state indefinitely.
|
@9larsons I addressed your comments and we reviewed in mob session and gave a 👍 so I've set to auto-merge pending your approval |
| await expect(staffDetailsPage.emailInput).toHaveValue(ghostAccountOwner.email); | ||
| await expect(staffDetailsPage.slugInput).toBeVisible(); | ||
|
|
||
| const currentUserSlug = await staffDetailsPage.slugInput.inputValue(); |
There was a problem hiding this comment.
ghostAccountOwner does not expose a slug so we go via the form details
3 participants
no issue
/ghost/#/my-profileto the signed-in users actual staff slug route in the settings app