Fixed missing onboarding flow in React admin shell#27625
Merged
kevinansfield merged 1 commit intomainfrom May 6, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (28)
✅ Files skipped from review due to trivial changes (10)
🚧 Files skipped from review as they are similar to previous changes (13)
WalkthroughThis pull request introduces a complete onboarding checklist feature to Ghost Admin. It adds user preference storage for onboarding state (including 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![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 (2)
apps/admin/src/onboarding/components/onboarding-checklist.tsx (1)
8-9: TightencompletedStepstyping to onboarding step ids.Line 8 should use
OnboardingStep[]so invalid ids are caught at compile-time instead of being ignored at runtime.🔧 Proposed refactor
interface OnboardingChecklistProps { allStepsCompleted: boolean; - completedSteps: string[]; + completedSteps: OnboardingStep[]; nextStep: OnboardingStep | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/onboarding/components/onboarding-checklist.tsx` around lines 8 - 9, The prop/field completedSteps is currently typed as string[] but should be tightened to OnboardingStep[] so invalid step ids are caught at compile time; update the type declaration for completedSteps to OnboardingStep[] (leaving nextStep: OnboardingStep | undefined as-is), then fix any call sites or state initializers that pass raw strings (convert them to OnboardingStep values or cast explicitly where appropriate) and ensure OnboardingStep is imported/available in the module so the component/props signature (e.g., the OnboardingChecklist props or interface) compiles.e2e/tests/admin/onboarding.test.ts (1)
95-105: Split this into two tests.This case exercises both the
completedanddismissedpaths, so one failure obscures which onboarding state regressed.As per coding guidelines, "One test should equal one scenario - never mix multiple scenarios in a single test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/onboarding.test.ts` around lines 95 - 105, Split the combined test into two separate tests: one that sets onboarding to 'completed' and verifies AnalyticsOverviewPage header is visible, and a second that sets onboarding to 'dismissed' and verifies the same; locate the original test named "completed and dismissed users reach Analytics normally" and replace it with two tests that call setOnboardingState(page, 'completed', allSteps) then await analyticsPage.goto() and expect(analyticsPage.header).toBeVisible() in the first, and setOnboardingState(page, 'dismissed') then await analyticsPage.goto() and expect(analyticsPage.header).toBeVisible() in the second, keeping use of AnalyticsOverviewPage and analyticsPage consistent.
🤖 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/onboarding/components/onboarding-logo-video.tsx`:
- Around line 7-31: The two <video> elements in onboarding-logo-video.tsx are
decorative and should be hidden from assistive tech; update both video elements
(the one sourcing logoLoaderUrl and the one sourcing logoLoaderDarkUrl) to
include aria-hidden="true" and role="presentation" (optionally tabIndex={-1}) so
screen readers ignore them; ensure you add these attributes on the <video> nodes
within the OnboardingLogoVideo component.
In `@apps/admin/src/onboarding/onboarding-route.tsx`:
- Around line 30-33: The share step is being marked complete before the real
site URL resolves because siteUrl falls back to "/" while the site query is
pending; update handleStepClick (the function that marks "share-publication"
complete) to detect whether the real site data is available (e.g., check
site.data?.site?.url or that siteUrl !== "/") and only mark the step complete
after the real URL is present, or alternatively disable the share action until
site.data?.site?.url is truthy; adjust any UI that opens the share dialog so it
uses the resolved siteUrl after that check and do the same fix for the other
share-related code paths referenced (the other occurrences around the 62-67 and
94-100 blocks) so copy/share always use the real site URL.
- Around line 54-60: navigateAfterUpdate currently sets isLeavingRef.current and
setIsLeaving(true) before awaiting update(), but if update() (e.g.,
completeChecklist or dismissChecklist) rejects those flags stay set and the
route shows blank; wrap the await update() in a try/catch (or try/finally) so
that on rejection you reset isLeavingRef.current = false and setIsLeaving(false)
and then rethrow or handle the error appropriately, referencing
navigateAfterUpdate, isLeavingRef.current, setIsLeaving and the callers
completeChecklist / dismissChecklist to locate the change.
---
Nitpick comments:
In `@apps/admin/src/onboarding/components/onboarding-checklist.tsx`:
- Around line 8-9: The prop/field completedSteps is currently typed as string[]
but should be tightened to OnboardingStep[] so invalid step ids are caught at
compile time; update the type declaration for completedSteps to OnboardingStep[]
(leaving nextStep: OnboardingStep | undefined as-is), then fix any call sites or
state initializers that pass raw strings (convert them to OnboardingStep values
or cast explicitly where appropriate) and ensure OnboardingStep is
imported/available in the module so the component/props signature (e.g., the
OnboardingChecklist props or interface) compiles.
In `@e2e/tests/admin/onboarding.test.ts`:
- Around line 95-105: Split the combined test into two separate tests: one that
sets onboarding to 'completed' and verifies AnalyticsOverviewPage header is
visible, and a second that sets onboarding to 'dismissed' and verifies the same;
locate the original test named "completed and dismissed users reach Analytics
normally" and replace it with two tests that call setOnboardingState(page,
'completed', allSteps) then await analyticsPage.goto() and
expect(analyticsPage.header).toBeVisible() in the first, and
setOnboardingState(page, 'dismissed') then await analyticsPage.goto() and
expect(analyticsPage.header).toBeVisible() in the second, keeping use of
AnalyticsOverviewPage and analyticsPage consistent.
🪄 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: 31642cb1-856f-47b9-a4dd-f87d75a1acbb
⛔ Files ignored due to path filters (4)
apps/admin/src/assets/icons/social-facebook.svgis excluded by!**/*.svgapps/admin/src/assets/icons/social-linkedin.svgis excluded by!**/*.svgapps/admin/src/assets/icons/social-x.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
apps/admin/package.jsonapps/admin/src/hooks/user-preferences.test.tsxapps/admin/src/hooks/user-preferences.tsapps/admin/src/onboarding/analytics-onboarding-redirect.tsxapps/admin/src/onboarding/components/onboarding-checklist.tsxapps/admin/src/onboarding/components/onboarding-logo-video.tsxapps/admin/src/onboarding/components/onboarding-step-item.tsxapps/admin/src/onboarding/components/share-publication-dialog.tsxapps/admin/src/onboarding/constants.tsapps/admin/src/onboarding/hooks/use-onboarding.test.tsxapps/admin/src/onboarding/hooks/use-onboarding.tsapps/admin/src/onboarding/onboarding-route.tsxapps/admin/src/routes.tsxapps/admin/src/utils/deep-merge.tsapps/admin/src/vite-env.d.tsapps/admin/vite.config.tse2e/helpers/pages/admin/index.tse2e/helpers/pages/admin/onboarding/index.tse2e/helpers/pages/admin/onboarding/onboarding-page.tse2e/tests/admin/onboarding.test.tsghost/admin/app/routes/setup/done.js
kevinansfield
force-pushed
the
codex/ber-3587-onboarding-checklist
branch
from
cdb9ce9 to
e6ca069
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/admin/src/onboarding/onboarding-route.tsx (2)
54-60:⚠️ Potential issue | 🟠 MajorReset leaving flags when checklist update fails.
If
update()rejects,isLeavingRef.currentandisLeavingstay true and the route can remain blank. Add error handling to reset both flags on failure.Suggested fix
const navigateAfterUpdate = async (update: () => Promise<unknown>) => { const targetReturnTo = getCurrentSafeReturnTo(); isLeavingRef.current = true; setIsLeaving(true); - await update(); - window.location.hash = targetReturnTo; + try { + await update(); + window.location.hash = targetReturnTo; + } catch (error) { + isLeavingRef.current = false; + setIsLeaving(false); + throw error; + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/onboarding/onboarding-route.tsx` around lines 54 - 60, The navigateAfterUpdate function sets isLeavingRef.current and setIsLeaving(true) before awaiting update(), but if update() rejects those flags stay true; wrap the await/update/navigation in a try/catch so that on catch you reset isLeavingRef.current = false and call setIsLeaving(false) and then rethrow (or propagate) the error; keep the successful path setting window.location.hash = targetReturnTo unchanged. Reference navigateAfterUpdate, isLeavingRef.current, setIsLeaving, update, getCurrentSafeReturnTo, and window.location.hash when making the change.
33-33:⚠️ Potential issue | 🟠 MajorAvoid completing the share step while
siteUrlis still the placeholder.At Line 33
siteUrlcan be'/'before site data resolves, but Line 63 marks the share step complete immediately. This can persist completion with invalid share/copy targets in the dialog.Also applies to: 62-67, 94-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/onboarding/onboarding-route.tsx` at line 33, The share step is being marked complete while siteUrl can still be the placeholder "/", so change the logic that completes the share step (the call that currently marks the share step complete—e.g., the function or setter that completes the "share" step) to only run when siteUrl is a real URL (check site.data?.site.url exists and siteUrl !== "/" and is not empty). Locate the code that uses the siteUrl variable and the completion call (references to siteUrl and the share-step completion function) and guard that completion behind a condition (or disable the UI control) until site.data is resolved and siteUrl is valid. Ensure any paths that previously completed the share step early are updated to use the same guard.
🧹 Nitpick comments (1)
apps/admin/src/onboarding/analytics-onboarding-redirect.tsx (1)
13-15: Replacenullreturn with a loading shell to prevent layout flicker.Returning
nullduring onboarding loading (line 13-14) unmounts the entire analytics subtree, causing visible blanking or flicker. Use a lightweight loading placeholder instead to maintain layout stability whileisLoadingresolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/onboarding/analytics-onboarding-redirect.tsx` around lines 13 - 15, Replace the early return of null when onboarding.isLoading with a lightweight loading shell component to avoid unmounting the analytics subtree and layout flicker: inside the component in analytics-onboarding-redirect.tsx (the block checking onboarding.isLoading) render a minimal placeholder that preserves the container dimensions (for example a Skeleton, Spinner in a wrapper, or a div with the same height/width and aria-busy attribute) instead of returning null so the layout remains stable until onboarding.isLoading is false.
🤖 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/onboarding.test.ts`:
- Around line 79-93: The test currently named 'analytics routes redirect to
onboarding while active' bundles two scenarios; split it into two separate tests
each asserting one route: one test should call startOnboarding(page), navigate
to '/ghost/#/analytics', instantiate OnboardingPage and assert checklist
visibility and URL contains '/setup/onboarding?returnTo=%2Fanalytics'; the
second test should do the same but navigate to '/ghost/#/analytics/web' and
assert the returnTo is '%2Fanalytics%2Fweb'. Move the shared setup
(startOnboarding(page) and any OnboardingPage construction) into a beforeEach or
a small helper if needed, and give each new test a clear name reflecting the
single scenario.
- Around line 63-177: Several test titles don't follow the "what is tested -
expected outcome" lowercase format; rename the dynamic navigation test and the
share test and any other tests missing the " - " separator. In the
navigationSteps.forEach block change the test title from `${step} step marks
complete and navigates` to `${step} - marks complete and navigates` (update the
template in the test(...) call), and change the test call for the share step
from 'share step opens the dialog and marks the step complete' to 'share step -
opens the dialog and marks the step complete'; scan for other occurrences of
test(...) strings in this file that lack the " - " separator and update them to
the lowercase "what is tested - expected outcome" format.
---
Duplicate comments:
In `@apps/admin/src/onboarding/onboarding-route.tsx`:
- Around line 54-60: The navigateAfterUpdate function sets isLeavingRef.current
and setIsLeaving(true) before awaiting update(), but if update() rejects those
flags stay true; wrap the await/update/navigation in a try/catch so that on
catch you reset isLeavingRef.current = false and call setIsLeaving(false) and
then rethrow (or propagate) the error; keep the successful path setting
window.location.hash = targetReturnTo unchanged. Reference navigateAfterUpdate,
isLeavingRef.current, setIsLeaving, update, getCurrentSafeReturnTo, and
window.location.hash when making the change.
- Line 33: The share step is being marked complete while siteUrl can still be
the placeholder "/", so change the logic that completes the share step (the call
that currently marks the share step complete—e.g., the function or setter that
completes the "share" step) to only run when siteUrl is a real URL (check
site.data?.site.url exists and siteUrl !== "/" and is not empty). Locate the
code that uses the siteUrl variable and the completion call (references to
siteUrl and the share-step completion function) and guard that completion behind
a condition (or disable the UI control) until site.data is resolved and siteUrl
is valid. Ensure any paths that previously completed the share step early are
updated to use the same guard.
---
Nitpick comments:
In `@apps/admin/src/onboarding/analytics-onboarding-redirect.tsx`:
- Around line 13-15: Replace the early return of null when onboarding.isLoading
with a lightweight loading shell component to avoid unmounting the analytics
subtree and layout flicker: inside the component in
analytics-onboarding-redirect.tsx (the block checking onboarding.isLoading)
render a minimal placeholder that preserves the container dimensions (for
example a Skeleton, Spinner in a wrapper, or a div with the same height/width
and aria-busy attribute) instead of returning null so the layout remains stable
until onboarding.isLoading is false.
🪄 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: 43be2ba0-d539-41a8-bc10-73bc94b1abd6
⛔ Files ignored due to path filters (4)
apps/admin/src/assets/icons/social-facebook.svgis excluded by!**/*.svgapps/admin/src/assets/icons/social-linkedin.svgis excluded by!**/*.svgapps/admin/src/assets/icons/social-x.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
apps/admin/package.jsonapps/admin/src/hooks/user-preferences.test.tsxapps/admin/src/hooks/user-preferences.tsapps/admin/src/onboarding/analytics-onboarding-redirect.tsxapps/admin/src/onboarding/components/onboarding-checklist.tsxapps/admin/src/onboarding/components/onboarding-logo-video.tsxapps/admin/src/onboarding/components/onboarding-step-item.tsxapps/admin/src/onboarding/components/share-publication-dialog.tsxapps/admin/src/onboarding/constants.tsapps/admin/src/onboarding/hooks/use-onboarding.test.tsxapps/admin/src/onboarding/hooks/use-onboarding.tsapps/admin/src/onboarding/onboarding-route.tsxapps/admin/src/routes.tsxapps/admin/src/utils/deep-merge.tsapps/admin/src/vite-env.d.tsapps/admin/vite.config.tse2e/helpers/pages/admin/index.tse2e/helpers/pages/admin/onboarding/index.tse2e/helpers/pages/admin/onboarding/onboarding-page.tse2e/tests/admin/onboarding.test.tsghost/admin/app/routes/setup/done.jsghost/admin/mirage/config/authentication.jsghost/admin/tests/acceptance/onboarding-test.jsghost/admin/tests/acceptance/setup-test.js
✅ Files skipped from review due to trivial changes (5)
- e2e/helpers/pages/admin/onboarding/index.ts
- e2e/helpers/pages/admin/index.ts
- apps/admin/src/vite-env.d.ts
- apps/admin/src/utils/deep-merge.ts
- apps/admin/package.json
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/admin/vite.config.ts
- apps/admin/src/onboarding/hooks/use-onboarding.test.tsx
- e2e/helpers/pages/admin/onboarding/onboarding-page.ts
- apps/admin/src/onboarding/components/onboarding-logo-video.tsx
- apps/admin/src/routes.tsx
- apps/admin/src/onboarding/components/onboarding-step-item.tsx
- apps/admin/src/onboarding/hooks/use-onboarding.ts
- ghost/admin/app/routes/setup/done.js
Comment on lines
+63
to
+177
| test('new owner setup flow lands on onboarding', async ({page}) => { | ||
| await setOnboardingState(page, 'pending', ['customize-design']); | ||
|
|
||
| await page.goto('/ghost/#/setup/done'); | ||
|
|
||
| const onboardingPage = new OnboardingPage(page); | ||
| await expect(onboardingPage.checklist).toBeVisible(); | ||
| await expect(page).toHaveURL(/\/ghost\/#\/setup\/onboarding\?returnTo=(%2F|\/)analytics/); | ||
|
|
||
| const preferences = await getOnboardingPreferences(page); | ||
| expect(preferences).toMatchObject({ | ||
| checklistState: 'started', | ||
| completedSteps: [] | ||
| }); | ||
| }); | ||
|
|
||
| test('analytics routes redirect to onboarding while active', async ({page}) => { | ||
| await startOnboarding(page); | ||
|
|
||
| await page.goto('/ghost/#/analytics'); | ||
|
|
||
| let onboardingPage = new OnboardingPage(page); | ||
| await expect(onboardingPage.checklist).toBeVisible(); | ||
| await expect(page).toHaveURL(/\/ghost\/#\/setup\/onboarding\?returnTo=%2Fanalytics/); | ||
|
|
||
| await page.goto('/ghost/#/analytics/web'); | ||
|
|
||
| onboardingPage = new OnboardingPage(page); | ||
| await expect(onboardingPage.checklist).toBeVisible(); | ||
| await expect(page).toHaveURL(/\/ghost\/#\/setup\/onboarding\?returnTo=%2Fanalytics%2Fweb/); | ||
| }); | ||
|
|
||
| test('completed and dismissed users reach Analytics normally', async ({page}) => { | ||
| const analyticsPage = new AnalyticsOverviewPage(page); | ||
|
|
||
| await setOnboardingState(page, 'completed', allSteps); | ||
| await analyticsPage.goto(); | ||
| await expect(analyticsPage.header).toBeVisible(); | ||
|
|
||
| await setOnboardingState(page, 'dismissed'); | ||
| await analyticsPage.goto(); | ||
| await expect(analyticsPage.header).toBeVisible(); | ||
| }); | ||
|
|
||
| test('pending users reach Analytics normally and are not started by the React route', async ({page}) => { | ||
| const analyticsPage = new AnalyticsOverviewPage(page); | ||
|
|
||
| await setOnboardingState(page, 'pending', ['customize-design']); | ||
| await analyticsPage.goto(); | ||
| await expect(analyticsPage.header).toBeVisible(); | ||
|
|
||
| await page.goto('/ghost/#/setup/onboarding?returnTo=%2Fanalytics%3Fsource%3Dweb'); | ||
| await expect(page).toHaveURL(/\/ghost\/#\/analytics\?source=web$/); | ||
|
|
||
| const preferences = await getOnboardingPreferences(page); | ||
| expect(preferences).toMatchObject({ | ||
| checklistState: 'pending', | ||
| completedSteps: ['customize-design'] | ||
| }); | ||
| }); | ||
|
|
||
| navigationSteps.forEach(([step, expectedUrl]) => { | ||
| test(`${step} step marks complete and navigates`, async ({page}) => { | ||
| await startOnboarding(page); | ||
|
|
||
| const onboardingPage = new OnboardingPage(page); | ||
| await onboardingPage.step(step).click(); | ||
|
|
||
| await expect(page).toHaveURL(expectedUrl); | ||
|
|
||
| const preferences = await getOnboardingPreferences(page); | ||
| expect(preferences.completedSteps).toContain(step); | ||
| }); | ||
| }); | ||
|
|
||
| test('share step opens the dialog and marks the step complete', async ({page}) => { | ||
| await startOnboarding(page); | ||
|
|
||
| const onboardingPage = new OnboardingPage(page); | ||
| await onboardingPage.step('share-publication').click(); | ||
|
|
||
| await expect(onboardingPage.shareModal).toBeVisible(); | ||
|
|
||
| const preferences = await getOnboardingPreferences(page); | ||
| expect(preferences.completedSteps).toContain('share-publication'); | ||
| }); | ||
|
|
||
| test('skip returns to the preserved analytics URL', async ({page}) => { | ||
| await startOnboarding(page); | ||
| await page.goto('/ghost/#/analytics?source=web'); | ||
|
|
||
| const onboardingPage = new OnboardingPage(page); | ||
| await expect(page).toHaveURL(/\/ghost\/#\/setup\/onboarding\?returnTo=%2Fanalytics%3Fsource%3Dweb/); | ||
| await onboardingPage.skipButton.click(); | ||
|
|
||
| await expect(page).toHaveURL(/\/ghost\/#\/analytics\?source=web$/); | ||
|
|
||
| const preferences = await getOnboardingPreferences(page); | ||
| expect(preferences.checklistState).toBe('dismissed'); | ||
| }); | ||
|
|
||
| test('completing all steps returns to the preserved analytics URL', async ({page}) => { | ||
| await startOnboarding(page); | ||
| await setOnboardingState(page, 'started', allSteps); | ||
| await page.goto('/ghost/#/setup/onboarding?returnTo=%2Fanalytics%3Fsource%3Dweb'); | ||
|
|
||
| const onboardingPage = new OnboardingPage(page); | ||
| await expect(page).toHaveURL(/\/ghost\/#\/setup\/onboarding\?returnTo=%2Fanalytics%3Fsource%3Dweb/); | ||
| await onboardingPage.completeButton.click(); | ||
|
|
||
| await expect(page).toHaveURL(/\/ghost\/#\/analytics\?source=web$/); | ||
|
|
||
| const preferences = await getOnboardingPreferences(page); | ||
| expect(preferences.checklistState).toBe('completed'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Normalize test names to the required lowercase what is tested - expected outcome format.
Several test names (for example at Line 95 and Line 107) don’t follow the required lowercase format with the - separator. Please rename consistently for readability and standardization.
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/onboarding.test.ts` around lines 63 - 177, Several test
titles don't follow the "what is tested - expected outcome" lowercase format;
rename the dynamic navigation test and the share test and any other tests
missing the " - " separator. In the navigationSteps.forEach block change the
test title from `${step} step marks complete and navigates` to `${step} - marks
complete and navigates` (update the template in the test(...) call), and change
the test call for the share step from 'share step opens the dialog and marks the
step complete' to 'share step - opens the dialog and marks the step complete';
scan for other occurrences of test(...) strings in this file that lack the " - "
separator and update them to the lowercase "what is tested - expected outcome"
format.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27625 +/- ##
==========================================
- Coverage 73.10% 73.10% -0.01%
==========================================
Files 1559 1559
Lines 126514 126527 +13
Branches 15329 15339 +10
==========================================
+ Hits 92493 92497 +4
- Misses 33061 33069 +8
- Partials 960 961 +1
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:
|
kevinansfield
force-pushed
the
codex/ber-3587-onboarding-checklist
branch
from
e6ca069 to
aee2da1
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ghost/admin/tests/acceptance/onboarding-test.js (2)
52-53: Redundant assertion afterwaitUntil.The
expecton Line 53 is redundant sincewaitUntilalready guarantees the condition is met before resolving.♻️ Proposed simplification
await waitUntil(() => window.location.hash === '#/setup/onboarding?returnTo=/analytics'); - expect(window.location.hash).to.equal('#/setup/onboarding?returnTo=/analytics');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/tests/acceptance/onboarding-test.js` around lines 52 - 53, Remove the redundant assertion after waitUntil: the call to waitUntil(() => window.location.hash === '#/setup/onboarding?returnTo=/analytics') already guarantees the condition, so delete the subsequent expect(window.location.hash).to.equal('#/setup/onboarding?returnTo=/analytics');; keep only the waitUntil call to assert navigation via the existing helper (referencing waitUntil and window.location.hash).
82-83: Same redundant assertion pattern.Same as above - the
waitUntilalready validates the condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/tests/acceptance/onboarding-test.js` around lines 82 - 83, The test contains a redundant assertion: the await waitUntil(() => window.location.hash === '#/setup/onboarding?returnTo=/analytics') already validates the hash, so remove the duplicate expect(window.location.hash).to.equal('#/setup/onboarding?returnTo=/analytics') assertion; keep the waitUntil call (or if you prefer an explicit check, assert the condition inside the waitUntil callback) and delete the redundant expect line to avoid duplicate checks in onboarding-test.js.ghost/admin/app/routes/setup/done.js (1)
12-21: Consider adding error handling forstartChecklist()failure.If
startChecklist()rejects, the navigation to the onboarding route still occurs (Line 20), but the checklist state won't be set to "started". This could result in the user landing on the onboarding route in an unexpected state.🛡️ Proposed defensive handling
async beforeModel() { await super.beforeModel(...arguments); if (this.session.user.isOwnerOnly) { - await this.onboarding.startChecklist(); + try { + await this.onboarding.startChecklist(); + } catch (error) { + // Log error but continue - React route handles pending state + console.error('Failed to start onboarding checklist:', error); + } } if (this.session.user?.isAdmin) { window.location.hash = '/setup/onboarding?returnTo=/analytics'; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/app/routes/setup/done.js` around lines 12 - 21, The call to onboarding.startChecklist() in beforeModel can reject and currently the code proceeds to set window.location.hash for onboarding; wrap the onboarding.startChecklist() invocation in a try/catch inside beforeModel (only when session.user.isOwnerOnly is true), log or handle the error (e.g., processLogger.warn/error or this.onboarding.markFailed) and only proceed to set window.location.hash (or set a fallback state) if startChecklist() succeeded; reference the beforeModel method, onboarding.startChecklist, session.user.isOwnerOnly, session.user?.isAdmin and window.location.hash when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/admin/app/routes/setup/done.js`:
- Around line 12-21: The call to onboarding.startChecklist() in beforeModel can
reject and currently the code proceeds to set window.location.hash for
onboarding; wrap the onboarding.startChecklist() invocation in a try/catch
inside beforeModel (only when session.user.isOwnerOnly is true), log or handle
the error (e.g., processLogger.warn/error or this.onboarding.markFailed) and
only proceed to set window.location.hash (or set a fallback state) if
startChecklist() succeeded; reference the beforeModel method,
onboarding.startChecklist, session.user.isOwnerOnly, session.user?.isAdmin and
window.location.hash when applying the change.
In `@ghost/admin/tests/acceptance/onboarding-test.js`:
- Around line 52-53: Remove the redundant assertion after waitUntil: the call to
waitUntil(() => window.location.hash ===
'#/setup/onboarding?returnTo=/analytics') already guarantees the condition, so
delete the subsequent
expect(window.location.hash).to.equal('#/setup/onboarding?returnTo=/analytics');;
keep only the waitUntil call to assert navigation via the existing helper
(referencing waitUntil and window.location.hash).
- Around line 82-83: The test contains a redundant assertion: the await
waitUntil(() => window.location.hash ===
'#/setup/onboarding?returnTo=/analytics') already validates the hash, so remove
the duplicate
expect(window.location.hash).to.equal('#/setup/onboarding?returnTo=/analytics')
assertion; keep the waitUntil call (or if you prefer an explicit check, assert
the condition inside the waitUntil callback) and delete the redundant expect
line to avoid duplicate checks in onboarding-test.js.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2db16388-9c19-4102-8a9d-8c8fdb0f506a
⛔ Files ignored due to path filters (4)
apps/admin/src/assets/icons/social-facebook.svgis excluded by!**/*.svgapps/admin/src/assets/icons/social-linkedin.svgis excluded by!**/*.svgapps/admin/src/assets/icons/social-x.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
apps/admin/package.jsonapps/admin/src/hooks/user-preferences.test.tsxapps/admin/src/hooks/user-preferences.tsapps/admin/src/onboarding/analytics-onboarding-redirect.tsxapps/admin/src/onboarding/components/onboarding-checklist.tsxapps/admin/src/onboarding/components/onboarding-logo-video.tsxapps/admin/src/onboarding/components/onboarding-step-item.tsxapps/admin/src/onboarding/components/share-publication-dialog.tsxapps/admin/src/onboarding/constants.tsapps/admin/src/onboarding/hooks/use-onboarding.test.tsxapps/admin/src/onboarding/hooks/use-onboarding.tsapps/admin/src/onboarding/onboarding-route.tsxapps/admin/src/routes.tsxapps/admin/src/utils/deep-merge.tsapps/admin/src/vite-env.d.tsapps/admin/vite.config.tse2e/helpers/pages/admin/index.tse2e/helpers/pages/admin/onboarding/index.tse2e/helpers/pages/admin/onboarding/onboarding-page.tse2e/tests/admin/onboarding.test.tsghost/admin/app/routes/setup/done.jsghost/admin/mirage/config/authentication.jsghost/admin/tests/acceptance/onboarding-test.jsghost/admin/tests/acceptance/setup-test.js
✅ Files skipped from review due to trivial changes (7)
- e2e/helpers/pages/admin/onboarding/index.ts
- e2e/helpers/pages/admin/index.ts
- apps/admin/src/vite-env.d.ts
- apps/admin/src/utils/deep-merge.ts
- e2e/helpers/pages/admin/onboarding/onboarding-page.ts
- apps/admin/src/onboarding/components/onboarding-logo-video.tsx
- apps/admin/src/hooks/user-preferences.test.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/admin/package.json
- apps/admin/src/routes.tsx
- apps/admin/vite.config.ts
- ghost/admin/mirage/config/authentication.js
- apps/admin/src/onboarding/analytics-onboarding-redirect.tsx
- apps/admin/src/onboarding/hooks/use-onboarding.test.tsx
- apps/admin/src/onboarding/components/share-publication-dialog.tsx
- apps/admin/src/onboarding/components/onboarding-checklist.tsx
- ghost/admin/tests/acceptance/setup-test.js
- apps/admin/src/onboarding/hooks/use-onboarding.ts
kevinansfield
force-pushed
the
codex/ber-3587-onboarding-checklist
branch
from
aee2da1 to
e0eda7c
Compare
kevinansfield
changed the title
kevinansfield
force-pushed
the
codex/ber-3587-onboarding-checklist
branch
from
e0eda7c to
2a850ea
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/shade/src/components/features/share-modal/share-modal.stories.tsx (3)
265-272: ⚡ Quick win
ControlledPublicationdeclares args but render ignores them
argsare defined but never passed intoShareModal, so controls won’t affect this story.Suggested patch
-export const ControlledPublication: Story = { +export const ControlledPublication: Story = { ... - render: () => { + render: (args) => { const ControlledExample = () => { const [isOpen, setIsOpen] = useState(false); return ( <ShareModal {...publicationArgs} + {...args} open={isOpen} onClose={() => setIsOpen(false)} onOpenChange={setIsOpen} >Also applies to: 280-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/components/features/share-modal/share-modal.stories.tsx` around lines 265 - 272, The ControlledPublication story defines args but the story's render doesn't use them, so interactive controls have no effect; update the ControlledPublication story (and the other similar story around the 280-299 block) to accept args in its render and pass them into ShareModal (e.g., change the render to take (args) and spread or forward args to ShareModal: pass copyURL and preview from args or use <ShareModal {...args} />) so the Storybook controls drive the component props.
6-17: ⚡ Quick winAdd a short docs overview at the story meta level
The stories cover variants well, but the file is missing a brief component overview in story docs metadata.
As per coding guidelines: "Story content should include a short overview (what the component does and primary use case) and demonstrate key variants and states."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/components/features/share-modal/share-modal.stories.tsx` around lines 6 - 17, The story meta (the meta object for ShareModal) is missing a short docs overview; add a concise component description by setting parameters.docs.description.component on the meta for ShareModal that briefly explains what the ShareModal does and its primary use case (e.g., purpose, when to use it) so Storybook autodocs show the overview alongside the variants.
4-4: ⚡ Quick winUse the
@alias for internal importsThis story uses a relative internal import for
ShareModal; switch it to the alias form to match repository conventions.Suggested patch
-import ShareModal, {type ShareModalSocialLink} from './share-modal'; +import ShareModal, {type ShareModalSocialLink} from '@/components/features/share-modal/share-modal';As per coding guidelines: "Use the
@alias for internal imports (e.g.,@/lib/utils)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/shade/src/components/features/share-modal/share-modal.stories.tsx` at line 4, Replace the relative import of ShareModal and ShareModalSocialLink (currently: import ShareModal, {type ShareModalSocialLink} from './share-modal') with the repository @ alias form (use import ShareModal, { type ShareModalSocialLink } from '@/...') so the module is imported via the internal alias instead of a relative path, preserving the exact exported names and types.
🤖 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/shade/src/components/features/post-share-modal/post-share-modal.tsx`:
- Around line 69-72: The Close button in PostShareModal is using type="submit"
which can submit a surrounding form; update the Button in the footerAction (the
Close action that calls onClose) to use type="button" instead so it acts purely
as a click handler and won't trigger form submission in the PostShareModal
component.
In `@apps/shade/src/components/features/share-modal/share-modal.tsx`:
- Around line 49-69: Modify copyTextToClipboard to return a boolean success
flag: when navigator.clipboard.writeText succeeds return true; catch and on
failure continue to fallback and return true only if
document.execCommand('copy') actually succeeds, otherwise return false. Then
update the caller handleCopyLink (and the other caller around the share modal
copy actions) to await copyTextToClipboard and set isCopied = true only when the
returned value is true; if false, do not set copied-state and optionally
handle/report the failure. Ensure all code paths in copyTextToClipboard
explicitly return true/false and that exceptions are caught so callers receive a
reliable success result.
- Around line 135-136: The component defines onClose = () => {} as a prop and
currently relies on that callback to close the dialog, which is a no-op by
default; wrap both buttons that currently call onClose with
DialogPrimitive.Close asChild so Radix will always close the dialog when they
are clicked, keep calling onClose inside the button's onClick for side-effects,
and ensure DialogPrimitive is imported from `@radix-ui/react-dialog`; specifically
update the two close-button elements in the ShareModal component to be children
of <DialogPrimitive.Close asChild> while preserving any existing onClick
handlers that invoke onClose.
---
Nitpick comments:
In `@apps/shade/src/components/features/share-modal/share-modal.stories.tsx`:
- Around line 265-272: The ControlledPublication story defines args but the
story's render doesn't use them, so interactive controls have no effect; update
the ControlledPublication story (and the other similar story around the 280-299
block) to accept args in its render and pass them into ShareModal (e.g., change
the render to take (args) and spread or forward args to ShareModal: pass copyURL
and preview from args or use <ShareModal {...args} />) so the Storybook controls
drive the component props.
- Around line 6-17: The story meta (the meta object for ShareModal) is missing a
short docs overview; add a concise component description by setting
parameters.docs.description.component on the meta for ShareModal that briefly
explains what the ShareModal does and its primary use case (e.g., purpose, when
to use it) so Storybook autodocs show the overview alongside the variants.
- Line 4: Replace the relative import of ShareModal and ShareModalSocialLink
(currently: import ShareModal, {type ShareModalSocialLink} from './share-modal')
with the repository @ alias form (use import ShareModal, { type
ShareModalSocialLink } from '@/...') so the module is imported via the internal
alias instead of a relative path, preserving the exact exported names and types.
🪄 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: eae1124f-1554-4c39-ae07-bc188436f7d3
📒 Files selected for processing (27)
apps/admin/src/hooks/user-preferences.test.tsxapps/admin/src/hooks/user-preferences.tsapps/admin/src/onboarding/analytics-onboarding-redirect.tsxapps/admin/src/onboarding/components/onboarding-checklist.tsxapps/admin/src/onboarding/components/onboarding-logo-video.tsxapps/admin/src/onboarding/components/onboarding-step-item.tsxapps/admin/src/onboarding/components/share-publication-dialog.tsxapps/admin/src/onboarding/constants.tsapps/admin/src/onboarding/hooks/use-onboarding.test.tsxapps/admin/src/onboarding/hooks/use-onboarding.tsapps/admin/src/onboarding/onboarding-route.tsxapps/admin/src/routes.tsxapps/admin/src/utils/deep-merge.tsapps/shade/.storybook/preview.tsxapps/shade/src/components/features/post-share-modal/post-share-modal.tsxapps/shade/src/components/features/share-modal/index.tsapps/shade/src/components/features/share-modal/share-modal.stories.tsxapps/shade/src/components/features/share-modal/share-modal.tsxapps/shade/src/patterns.tse2e/helpers/pages/admin/index.tse2e/helpers/pages/admin/onboarding/index.tse2e/helpers/pages/admin/onboarding/onboarding-page.tse2e/tests/admin/onboarding.test.tsghost/admin/app/routes/setup/done.jsghost/admin/mirage/config/authentication.jsghost/admin/tests/acceptance/onboarding-test.jsghost/admin/tests/acceptance/setup-test.js
✅ Files skipped from review due to trivial changes (12)
- e2e/helpers/pages/admin/index.ts
- e2e/helpers/pages/admin/onboarding/index.ts
- apps/shade/src/components/features/share-modal/index.ts
- apps/shade/src/patterns.ts
- apps/admin/src/onboarding/components/onboarding-logo-video.tsx
- apps/admin/src/onboarding/analytics-onboarding-redirect.tsx
- apps/admin/src/hooks/user-preferences.ts
- e2e/helpers/pages/admin/onboarding/onboarding-page.ts
- apps/admin/src/onboarding/components/onboarding-step-item.tsx
- apps/admin/src/onboarding/constants.ts
- apps/admin/src/onboarding/onboarding-route.tsx
- ghost/admin/tests/acceptance/onboarding-test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/admin/src/utils/deep-merge.ts
- ghost/admin/app/routes/setup/done.js
- apps/admin/src/onboarding/components/share-publication-dialog.tsx
- apps/admin/src/hooks/user-preferences.test.tsx
Comment on lines
+135
to
+136
| onClose = () => {}, | ||
| preview, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @radix-ui/react-dialog, does clicking a regular button inside DialogContent close the dialog automatically, or is Dialog.Close required?
💡 Result:
Clicking a regular button inside DialogContent does not close the dialog automatically. Dialog.Close is required to close the dialog.
Citations:
- 1: https://www.radix-ui.com/docs/primitives/components/dialog
- 2: https://www.radix-ui.com/primitives/docs/components/dialog
- 3: https://radix-ui.com/primitives/docs/components/dialog
🏁 Script executed:
fd -t f "share-modal.tsx" apps/shade/src/Repository: TryGhost/Ghost
Length of output: 194
🏁 Script executed:
cat -n apps/shade/src/components/features/share-modal/share-modal.tsx | head -200Repository: TryGhost/Ghost
Length of output: 11910
🏁 Script executed:
cat -n apps/shade/src/components/features/share-modal/share-modal.tsx | sed -n '200,280p'Repository: TryGhost/Ghost
Length of output: 3192
Wrap close buttons with DialogPrimitive.Close to ensure dialog closes regardless of onClose callback
Close buttons at lines 158 and 175 rely solely on onClose for closing, which defaults to a no-op function. This makes dialog closure dependent on consumer implementation. In Radix UI's @radix-ui/react-dialog, clicking a regular button inside DialogContent does not automatically close the dialog—DialogPrimitive.Close is required. Wrap both close buttons with DialogPrimitive.Close asChild so the dialog always closes, keeping onClose as an optional side-effect callback.
Suggested patch
- <Button className="absolute -top-5 -right-5 cursor-pointer p-2 text-muted-foreground hover:text-foreground [&_svg]:size-6!" id={closeButtonId} size="lg" title="Close" variant="link" onClick={onClose}>
- <X size={24} strokeWidth={1} />
- <span className="sr-only">Close</span>
- </Button>
+ <DialogPrimitive.Close asChild>
+ <Button className="absolute -top-5 -right-5 cursor-pointer p-2 text-muted-foreground hover:text-foreground [&_svg]:size-6!" id={closeButtonId} size="lg" title="Close" variant="link" onClick={onClose}>
+ <X size={24} strokeWidth={1} />
+ <span className="sr-only">Close</span>
+ </Button>
+ </DialogPrimitive.Close>
...
- <Button className="-mr-2 cursor-pointer p-2 text-muted-foreground hover:text-foreground [&_svg]:size-6!" id={closeButtonId} size="lg" title="Close" variant="link" onClick={onClose}>
- <X size={24} strokeWidth={1} />
- <span className="sr-only">Close</span>
- </Button>
+ <DialogPrimitive.Close asChild>
+ <Button className="-mr-2 cursor-pointer p-2 text-muted-foreground hover:text-foreground [&_svg]:size-6!" id={closeButtonId} size="lg" title="Close" variant="link" onClick={onClose}>
+ <X size={24} strokeWidth={1} />
+ <span className="sr-only">Close</span>
+ </Button>
+ </DialogPrimitive.Close>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onClose = () => {}, | |
| preview, | |
| <DialogPrimitive.Close asChild> | |
| <Button className="absolute -top-5 -right-5 cursor-pointer p-2 text-muted-foreground hover:text-foreground [&_svg]:size-6!" id={closeButtonId} size="lg" title="Close" variant="link" onClick={onClose}> | |
| <X size={24} strokeWidth={1} /> | |
| <span className="sr-only">Close</span> | |
| </Button> | |
| </DialogPrimitive.Close> |
Suggested change
| onClose = () => {}, | |
| preview, | |
| <DialogPrimitive.Close asChild> | |
| <Button className="-mr-2 cursor-pointer p-2 text-muted-foreground hover:text-foreground [&_svg]:size-6!" id={closeButtonId} size="lg" title="Close" variant="link" onClick={onClose}> | |
| <X size={24} strokeWidth={1} /> | |
| <span className="sr-only">Close</span> | |
| </Button> | |
| </DialogPrimitive.Close> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/shade/src/components/features/share-modal/share-modal.tsx` around lines
135 - 136, The component defines onClose = () => {} as a prop and currently
relies on that callback to close the dialog, which is a no-op by default; wrap
both buttons that currently call onClose with DialogPrimitive.Close asChild so
Radix will always close the dialog when they are clicked, keep calling onClose
inside the button's onClick for side-effects, and ensure DialogPrimitive is
imported from `@radix-ui/react-dialog`; specifically update the two close-button
elements in the ShareModal component to be children of <DialogPrimitive.Close
asChild> while preserving any existing onClick handlers that invoke onClose.
62c796f to
a2439ed
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
e2e/tests/admin/onboarding.test.ts (1)
67-197:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize test titles to the required lowercase
what is tested - expected outcomeformatSeveral titles still miss the required
-pattern and/or lowercase normalization (for example, Line 145 and Line 158). Please rename all test titles in this file to the standard format consistently.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/onboarding.test.ts` around lines 67 - 197, Several test titles in e2e/tests/admin/onboarding.test.ts do not follow the required lowercase "what is tested - expected outcome" format with a " - " separator; update every test(...) title string (e.g., the titles for the tests that start with "completed and dismissed users reach Analytics normally", "pending users reach Analytics normally and are not started by the React route", "legacy started users without startedAt reach Analytics and are dismissed", and the navigation/step/share/skip/complete tests) to a normalized lowercase form that uses " - " between the subject and expected outcome (for example: "completed and dismissed users reach analytics - should access analytics normally" or similar), ensuring all titles in functions like test(...) and in the navigationSteps.forEach generated test names follow the same pattern.
🤖 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/onboarding/hooks/use-onboarding.ts`:
- Around line 63-66: The retry guard
hasAttemptedInvalidStartedStateDismissalRef.current is set before calling the
async dismissChecklist(), so if dismissChecklist() rejects the guard is never
reset and retries are permanently blocked; update the logic around
dismissChecklist() (the call and its Promise handling) so the ref is only set
when the dismissal succeeds (or reset to false inside the .catch handler) —
locate the hasAttemptedInvalidStartedStateDismissalRef usage and the
dismissChecklist() call and either move the assignment to after a successful
await/then or ensure the catch handler resets the ref so users can retry.
In `@apps/shade/src/components/features/post-share-modal/post-share-modal.tsx`:
- Around line 29-30: The footer "Close" button currently only calls the optional
onClose prop (defaulting to a no-op) which makes it non-functional when callers
omit onClose (especially in emailOnly mode); change the footer handler so it
always closes the modal via the component's internal close routine (e.g., call a
local closeModal/handleClose that updates modal state or triggers the parent
close mechanism) and then call onClose if it's provided. Locate the onClose prop
default and the footer Close click handler (the code invoking onClose around the
footer/emailOnly area) and refactor: implement a local close handler
(closeModal/handleClose) that performs the actual close logic, wire the footer
button to that handler, and have that handler conditionally call the onClose
callback if present.
---
Duplicate comments:
In `@e2e/tests/admin/onboarding.test.ts`:
- Around line 67-197: Several test titles in e2e/tests/admin/onboarding.test.ts
do not follow the required lowercase "what is tested - expected outcome" format
with a " - " separator; update every test(...) title string (e.g., the titles
for the tests that start with "completed and dismissed users reach Analytics
normally", "pending users reach Analytics normally and are not started by the
React route", "legacy started users without startedAt reach Analytics and are
dismissed", and the navigation/step/share/skip/complete tests) to a normalized
lowercase form that uses " - " between the subject and expected outcome (for
example: "completed and dismissed users reach analytics - should access
analytics normally" or similar), ensuring all titles in functions like test(...)
and in the navigationSteps.forEach generated test names follow the same pattern.
🪄 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: ba897268-61f8-41fb-a469-0ba9c76e74d7
📒 Files selected for processing (28)
apps/admin/src/hooks/user-preferences.test.tsxapps/admin/src/hooks/user-preferences.tsapps/admin/src/onboarding/analytics-onboarding-redirect.tsxapps/admin/src/onboarding/components/onboarding-checklist.tsxapps/admin/src/onboarding/components/onboarding-logo-video.tsxapps/admin/src/onboarding/components/onboarding-step-item.tsxapps/admin/src/onboarding/components/share-publication-dialog.tsxapps/admin/src/onboarding/constants.tsapps/admin/src/onboarding/hooks/use-onboarding.test.tsxapps/admin/src/onboarding/hooks/use-onboarding.tsapps/admin/src/onboarding/onboarding-route.tsxapps/admin/src/routes.tsxapps/admin/src/utils/deep-merge.tsapps/shade/.storybook/preview.tsxapps/shade/src/components/features/post-share-modal/post-share-modal.tsxapps/shade/src/components/features/share-modal/index.tsapps/shade/src/components/features/share-modal/share-modal.stories.tsxapps/shade/src/components/features/share-modal/share-modal.tsxapps/shade/src/patterns.tse2e/helpers/pages/admin/index.tse2e/helpers/pages/admin/onboarding/index.tse2e/helpers/pages/admin/onboarding/onboarding-page.tse2e/tests/admin/onboarding.test.tsghost/admin/app/routes/setup/done.jsghost/admin/app/services/onboarding.jsghost/admin/mirage/config/authentication.jsghost/admin/tests/acceptance/onboarding-test.jsghost/admin/tests/acceptance/setup-test.js
✅ Files skipped from review due to trivial changes (6)
- e2e/helpers/pages/admin/onboarding/index.ts
- e2e/helpers/pages/admin/index.ts
- apps/shade/src/components/features/share-modal/index.ts
- e2e/helpers/pages/admin/onboarding/onboarding-page.ts
- apps/admin/src/onboarding/onboarding-route.tsx
- apps/admin/src/onboarding/constants.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/shade/src/patterns.ts
- apps/admin/src/onboarding/analytics-onboarding-redirect.tsx
- apps/admin/src/utils/deep-merge.ts
- ghost/admin/mirage/config/authentication.js
- apps/admin/src/onboarding/components/share-publication-dialog.tsx
- apps/shade/src/components/features/share-modal/share-modal.tsx
- apps/shade/src/components/features/share-modal/share-modal.stories.tsx
- apps/admin/src/onboarding/hooks/use-onboarding.test.tsx
Comment on lines
+29
to
+30
| onClose = () => {}, | ||
| postExcerpt = '', |
Contributor
There was a problem hiding this comment.
Make the footer “Close” action independent of an optional callback.
At Line 29 onClose defaults to a no-op, and Lines 69-73 only invoke that callback. In emailOnly mode, this can leave the footer “Close” button non-functional when callers don’t pass onClose.
Suggested patch
const PostShareModal: React.FC<PostShareModalProps> = ({
@@
...props
}) => {
+ const handleClose = () => {
+ props.onOpenChange?.(false);
+ onClose();
+ };
+
@@
footerAction={emailOnly ? (
- <Button className="cursor-pointer" type="button" onClick={onClose}>
+ <Button className="cursor-pointer" type="button" onClick={handleClose}>
Close
</Button>
) : undefined}Also applies to: 69-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/shade/src/components/features/post-share-modal/post-share-modal.tsx`
around lines 29 - 30, The footer "Close" button currently only calls the
optional onClose prop (defaulting to a no-op) which makes it non-functional when
callers omit onClose (especially in emailOnly mode); change the footer handler
so it always closes the modal via the component's internal close routine (e.g.,
call a local closeModal/handleClose that updates modal state or triggers the
parent close mechanism) and then call onClose if it's provided. Locate the
onClose prop default and the footer Close click handler (the code invoking
onClose around the footer/emailOnly area) and refactor: implement a local close
handler (closeModal/handleClose) that performs the actual close logic, wire the
footer button to that handler, and have that handler conditionally call the
onClose callback if present.
jonatansberg
requested changes
May 4, 2026
Member
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
Wouldn't it make more sense to pull out the Ember side of this and only have the React implementation? I find the current state a bit confusing.
Even if we leave the Ember stuff in, there are still a few minor things I think we should clean up before merging.
Comment on lines
+13
to
+17
| export const DEFAULT_ONBOARDING_PREFERENCES = { | ||
| completedSteps: [] as string[], | ||
| checklistState: "pending" as const, | ||
| startedAt: undefined as Date | undefined, | ||
| }; |
Member
There was a problem hiding this comment.
Not blocking: I wonder if there is a better way for us to align this with the schema without the typecasts. Maybe we could just inline the defaults below and then parse an empty object to get the default onboarding preferences? 🤔
Comment on lines
+59
to
+66
| if (isUserLoading || isPreferencesLoading || !isOwner || checklistState !== "started" || hasActiveStartedAt || hasAttemptedInvalidStartedStateDismissalRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| hasAttemptedInvalidStartedStateDismissalRef.current = true; | ||
| void dismissChecklist().catch((error) => { | ||
| console.error(error); | ||
| }); |
Member
There was a problem hiding this comment.
What is this ref even doing? And why are we using an effect to trigger a remote state change rather than just doing it from e.g. the markStepCompleted callback?
Member
Author
There was a problem hiding this comment.
The effect is normalizing persisted state on read via the effect because we need to handle the situation of not unexpectedly showing onboarding to all the sites that were created and put into the "started" state whilst onboarding wasn't shown for the last 6 months or so. We're detecting that by having checklistState: "started" without a valid startedAt, which will exist before the user has clicked anything that would mark steps as completed.
The ref is there to avoid firing duplicate dismiss mutations while the hook re-renders with the same stale preference data. Without it, every render where the query still says started with no active startedAt can schedule another PUT. It's reset on rejection so a later render can retry, and once the mutation succeeds the preference state changes to dismissed, so the effect naturally stops.
|
|
||
| onboardingPage = new OnboardingPage(page); | ||
| await expect(onboardingPage.checklist).toBeVisible(); | ||
| await expect(page).toHaveURL(/\/ghost\/#\/setup\/onboarding\?returnTo=%2Fanalytics%2Fweb/); |
Member
There was a problem hiding this comment.
Nit: if we are repeatedly asserting the return URL, maybe we should have a helper to format things for us?
closes https://linear.app/ghost/issue/BER-3587/ The onboarding flow was lost during the Ember->React migration of the Admin shell. This recreates it in the React admin app on a dedicated route, while keeping Analytics as the return destination until the checklist is skipped or completed. Because onboarding has been missing for several months, existing owners may have stale onboarding preferences. This PR records `onboarding.startedAt` when `/setup/done` starts the checklist and only shows `started` checklists whose timestamp is on or after the fixed 30 April 2026 cutoff. Missing or older timestamps are dismissed so long-time product users are not suddenly forced into onboarding. - adds a dedicated React admin onboarding route at `/setup/onboarding` - stores checklist state through the user preferences hooks while keeping the legacy accessibility storage detail hidden - redirects active onboarding users away from Analytics until they skip or complete the checklist - updates Ember `/setup/done` so new owner setup is the only path that starts onboarding - records `onboarding.startedAt` and dismisses missing/legacy `startedAt` values before the fixed 30 April 2026 cutoff so existing long-time owners are not forced into onboarding - shares the Shade `ShareModal` base between the onboarding publication share dialog and `PostShareModal` - aligns publication and post share options, including X, Threads, Facebook, LinkedIn, and copy link - adds Shade Storybook coverage for the shared share modal and fixes Storybook dark-mode handling for portaled content - adds onboarding E2E coverage, including pending existing owners and legacy started owners reaching Analytics normally
kevinansfield
force-pushed
the
codex/ber-3587-onboarding-checklist
branch
from
a2439ed to
cd7668e
Compare
jonatansberg
approved these changes
May 5, 2026
Member
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
Tested everything locally, and we seem to be good!
kevinansfield
added a commit
that referenced
this pull request
May 6, 2026
ref https://linear.app/ghost/issue/BER-3587/ Follow-up to #27625 — the apps/admin build was failing with a TS2353 error because Radix's `DialogContentProps` type does not include an index signature for `data-*` attributes, so passing `data-testid` (or any `data-*` attr) as a literal in `contentProps` failed strict excess-property checks. - widened `ShareModal`'s `contentProps` type to additionally accept arbitrary `data-*` keys, matching how the attribute is consumed on the rendered DOM element - removed the vestigial `data-test-modal` attribute from the onboarding share dialog; nothing references it (the e2e helper uses the `data-testid` selector)
lujuldotcom
pushed a commit
to lujuldotcom/Ghost
that referenced
this pull request
May 6, 2026
ref https://linear.app/ghost/issue/BER-3587/ The onboarding flow was lost during the Ember->React migration of the Admin shell. This recreates it in the React admin app on a dedicated route, while keeping Analytics as the return destination until the checklist is skipped or completed. Because onboarding has been missing for several months, existing owners may have stale onboarding preferences. This PR records `onboarding.startedAt` when `/setup/done` starts the checklist and only shows `started` checklists whose timestamp is on or after the fixed 30 April 2026 cutoff. Missing or older timestamps are dismissed so long-time product users are not suddenly forced into onboarding. ## Summary - adds a dedicated React admin onboarding route at `/setup/onboarding` - stores checklist state through the user preferences hooks while keeping the legacy accessibility storage detail hidden - redirects active onboarding users away from Analytics until they skip or complete the checklist - updates Ember `/setup/done` so new owner setup is the only path that starts onboarding - records `onboarding.startedAt` and dismisses missing/legacy `startedAt` values before the fixed 30 April 2026 cutoff so existing long-time owners are not forced into onboarding - shares the Shade `ShareModal` base between the onboarding publication share dialog and `PostShareModal` - aligns publication and post share options, including X, Threads, Facebook, LinkedIn, and copy link - adds Shade Storybook coverage for the shared share modal and fixes Storybook dark-mode handling for portaled content - adds onboarding E2E coverage, including pending existing owners and legacy started owners reaching Analytics normally
lujuldotcom
pushed a commit
to lujuldotcom/Ghost
that referenced
this pull request
May 6, 2026
…st#27696) ref https://linear.app/ghost/issue/BER-3587/ Follow-up to TryGhost#27625 — the apps/admin build was failing with a TS2353 error because Radix's `DialogContentProps` type does not include an index signature for `data-*` attributes, so passing `data-testid` (or any `data-*` attr) as a literal in `contentProps` failed strict excess-property checks. - widened `ShareModal`'s `contentProps` type to additionally accept arbitrary `data-*` keys, matching how the attribute is consumed on the rendered DOM element - removed the vestigial `data-test-modal` attribute from the onboarding share dialog; nothing references it (the e2e helper uses the `data-testid` selector)
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
2 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.
ref https://linear.app/ghost/issue/BER-3587/new-signups-miss-the-onboarding-checklist
The onboarding flow was lost during the Ember->React migration of the Admin shell. This recreates it in the React admin app on a dedicated route, while keeping Analytics as the return destination until the checklist is skipped or completed.
Because onboarding has been missing for several months, existing owners may have stale onboarding preferences. This PR records
onboarding.startedAtwhen/setup/donestarts the checklist and only showsstartedchecklists whose timestamp is on or after the fixed 30 April 2026 cutoff. Missing or older timestamps are dismissed so long-time product users are not suddenly forced into onboarding.Summary
/setup/onboarding/setup/doneso new owner setup is the only path that starts onboardingonboarding.startedAtand dismisses missing/legacystartedAtvalues before the fixed 30 April 2026 cutoff so existing long-time owners are not forced into onboardingShareModalbase between the onboarding publication share dialog andPostShareModal