Added URL routing to staff modal email notification tab #25572
Conversation
WalkthroughThis pull request introduces email notification settings management for staff members. The changes update URL construction across email service files to append Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/general/UserDetailModal.tsx (1)
167-175: Consider movinggetTabFromPathoutside the component.This helper function doesn't depend on any component state or props (other than its parameter). Moving it outside
UserDetailModalContentwould avoid recreating it on every render and improve readability.+const getTabFromPath = (path: string): string => { + const lastSegment = path.split('/').pop() || ''; + + if (lastSegment === 'social-links' || lastSegment === 'email-notifications') { + return lastSegment; + } + + return 'profile'; +}; + const UserDetailModalContent: React.FC<{user: User; pathName: string}> = ({user, pathName}) => { const {updateRoute} = useRouting(); - - const getTabFromPath = (path: string): string => { - const lastSegment = path.split('/').pop() || ''; - - if (lastSegment === 'social-links' || lastSegment === 'email-notifications') { - return lastSegment; - } - - return 'profile'; - };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/admin-x-settings/src/components/providers/SettingsRouter.tsx(1 hunks)apps/admin-x-settings/src/components/settings/general/UserDetailModal.tsx(4 hunks)ghost/core/core/server/services/comments/CommentsServiceEmails.js(2 hunks)ghost/core/core/server/services/staff/StaffServiceEmails.js(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-30T17:13:26.190Z
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 25303
File: ghost/core/core/server/services/email-service/BatchSendingService.js:19-19
Timestamp: 2025-10-30T17:13:26.190Z
Learning: In ghost/core/core/server/services/email-service/BatchSendingService.js and similar files in the Ghost codebase, prefer using `{...options}` spread syntax without explicit guards like `...(options || {})` when spreading potentially undefined objects, as the maintainer prefers cleaner syntax over defensive patterns when the behavior is safe.
Applied to files:
ghost/core/core/server/services/staff/StaffServiceEmails.jsghost/core/core/server/services/comments/CommentsServiceEmails.js
📚 Learning: 2025-06-13T11:57:58.226Z
Learnt from: kevinansfield
Repo: TryGhost/Ghost PR: 23824
File: ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs:919-926
Timestamp: 2025-06-13T11:57:58.226Z
Learning: In `ghost/core/core/server/services/email-service/email-templates/partials/styles.hbs`, some style blocks (e.g., `.latest-post p` and `.latest-post p a`) still use the legacy colour `#73818c` on purpose; they are later overridden by `emailCustomization` feature rules, as noted by inline TODO comments. These occurrences should not be flagged as inconsistencies.
Applied to files:
ghost/core/core/server/services/comments/CommentsServiceEmails.js
🧬 Code graph analysis (1)
apps/admin-x-settings/src/components/settings/general/UserDetailModal.tsx (1)
apps/admin-x-framework/src/api/users.ts (1)
User(9-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (6)
ghost/core/core/server/services/comments/CommentsServiceEmails.js (2)
48-48: LGTM!The staffUrl correctly appends
/email-notificationsto direct post authors to the email notifications settings tab when they click the link in comment notification emails.
163-163: LGTM!The staffUrl for the site owner in report notifications correctly links to the email notifications tab, consistent with the change in
notifyPostAuthors.ghost/core/core/server/services/staff/StaffServiceEmails.js (1)
40-40: LGTM!All staffUrl constructions consistently append
/email-notificationsacross all notification methods. The pattern is uniform and aligns with the new frontend routing.Also applies to: 97-97, 152-152, 185-185, 230-230, 285-285
apps/admin-x-settings/src/components/providers/SettingsRouter.tsx (1)
12-15: LGTM!The route ordering is correct—more specific routes (
social-links,email-notifications,edit) are defined before the genericstaff/:slugcatch-all route, ensuring proper path matching.apps/admin-x-settings/src/components/settings/general/UserDetailModal.tsx (2)
415-423: LGTM!The tab initialization and routing synchronization logic is correct:
initialTabproperly derives the tab from the URL pathhandleTabChangecorrectly updates both the URL and local state, keeping them in sync- The URL construction correctly omits the segment for the 'profile' tab (default)
575-591: LGTM!The
pathNameprop is correctly threaded fromRoutingModalPropsthrough toUserDetailModalContent, enabling URL-based tab selection.
minimaluminium
force-pushed
the
email-preferences-link-BER-3080
branch
from
23f3aa0 to
994848f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ghost/core/test/e2e-api/webmentions/__snapshots__/webmentions.test.js.snapis excluded by!**/*.snapghost/core/test/e2e-server/services/__snapshots__/recommendation-emails.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
apps/admin-x-settings/src/components/providers/settings-router.tsx(1 hunks)apps/admin-x-settings/src/components/settings/general/user-detail-modal.tsx(3 hunks)ghost/core/core/server/services/comments/CommentsServiceEmails.js(2 hunks)ghost/core/core/server/services/staff/StaffServiceEmails.js(6 hunks)ghost/core/test/unit/server/services/staff/staff-service.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/core/server/services/comments/CommentsServiceEmails.js
- ghost/core/test/unit/server/services/staff/staff-service.test.js
- ghost/core/core/server/services/staff/StaffServiceEmails.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (3)
apps/admin-x-settings/src/components/settings/general/user-detail-modal.tsx (2)
165-175: Tab path parsing logic is sound.The
getTabFromPathfunction correctly extracts the last URL segment and maps it to valid tab identifiers, with a sensible default to 'profile'.
567-567: Tab change handler properly wired.The
onTabChangecallback correctly delegates tohandleTabChangeto keep the URL and selected tab in sync.apps/admin-x-settings/src/components/providers/settings-router.tsx (1)
12-15: Route additions are properly structured.The new tab routes for social-links and email-notifications are correctly placed before the catch-all
/staff/:slugroute, ensuring proper route matching precedence. All routes appropriately map toUserDetailModalto support the tab-driven navigation.
| const getTabFromPath = (path: string): string => { | ||
| const lastSegment = path.split('/').pop() || ''; | ||
|
|
||
| if (lastSegment === 'social-links' || lastSegment === 'email-notifications') { | ||
| return lastSegment; | ||
| } | ||
|
|
||
| return 'profile'; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts --type=tsx "staff/.*edit" -g '!*.test.*'Repository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# Also check for any route definitions and references to the edit route
rg -n "edit.*route\|route.*edit" apps/admin-x-settings/src --type=ts --type=tsxRepository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# Check the settings-router.tsx file to see the context of line 14
cat -n apps/admin-x-settings/src/components/settings/general/settings-router.tsx | head -20Repository: TryGhost/Ghost
Length of output: 163
🏁 Script executed:
# Search for any navigation to /staff/:slug/edit pattern
rg -n "navigate.*edit\|href.*edit" apps/admin-x-settings/src --type=ts --type=tsx -A 2Repository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# Find the correct paths for these files
fd -type f "settings-router\|user-detail-modal" --extension ts --extension tsxRepository: TryGhost/Ghost
Length of output: 228
🏁 Script executed:
# Search for staff and edit references more broadly
rg -n "staff.*edit" -A 2 -B 2 --max-count=20Repository: TryGhost/Ghost
Length of output: 3115
🏁 Script executed:
# Find all route definitions with staff
rg -n "staff.*:slug" -A 2 -B 2Repository: TryGhost/Ghost
Length of output: 1367
🏁 Script executed:
# Search for getTabFromPath function usage
rg -n "getTabFromPath" -A 3 -B 3Repository: TryGhost/Ghost
Length of output: 1751
🏁 Script executed:
# Search for navigation or links to the edit route
rg -n "staff.*edit\|/edit" apps/admin-x-settings/src/components/settings/general --max-count=30Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check if there are any comments or documentation about the edit route
rg -n "edit.*route\|TODO.*edit\|FIXME" apps/admin-x-settings/src/components/settings/general/user-detail-modal.tsxRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Search for all uses of updateRoute that might navigate to edit
rg -n "updateRoute.*edit" apps/admin-x-settings/src --max-count=20Repository: TryGhost/Ghost
Length of output: 1648
🏁 Script executed:
# Double-check by searching the entire codebase for any navigation to staff/:slug/edit
rg -n "staff/.*edit" --type-list | head -5Repository: TryGhost/Ghost
Length of output: 447
🏁 Script executed:
# Search all TypeScript/JS files for staff edit navigation
rg "staff.*edit" -g '*.ts' -g '*.tsx' -g '*.js' --no-headingRepository: TryGhost/Ghost
Length of output: 819
🏁 Script executed:
# Check if staff/:slug/edit is referenced in any test files or config
rg "staff.*:slug.*edit"Repository: TryGhost/Ghost
Length of output: 167
🏁 Script executed:
# Check when the staff/:slug/edit route was added and if there are any comments
git log --oneline -p apps/admin-x-settings/src/components/providers/settings-router.tsx | grep -A 5 -B 5 "staff/:slug/edit" | head -20Repository: TryGhost/Ghost
Length of output: 628
🏁 Script executed:
# See the full user-detail-modal.tsx to understand the complete picture
wc -l apps/admin-x-settings/src/components/settings/general/user-detail-modal.tsxRepository: TryGhost/Ghost
Length of output: 137
🏁 Script executed:
# Check what the base 'staff/:slug' route resolves to
cat apps/admin-x-settings/src/components/settings/general/user-detail-modal.tsx | grep -A 10 "const initialTab"Repository: TryGhost/Ghost
Length of output: 413
Remove the unused /staff/:slug/edit route or document its purpose.
The route 'staff/:slug/edit': 'UserDetailModal' defined in settings-router.tsx is never navigated to by the application. The getTabFromPath function doesn't recognize 'edit' as a segment, causing it to default to 'profile', but more importantly, no code in the codebase ever generates navigation to this URL. The handleTabChange function navigates to either the base staff/{slug} (for profile tab) or staff/{slug}/{tab} (for other tabs), but never includes an 'edit' segment. This route appears to be dead code and should either be removed or its purpose documented if it exists for backwards compatibility.
🤖 Prompt for AI Agents
In apps/admin-x-settings/src/components/settings/general/user-detail-modal.tsx
around lines 167 to 175, the codebase exposes a dead route 'staff/:slug/edit'
(settings-router.tsx) that is never navigated to and isn't recognized by
getTabFromPath (so it falls back to 'profile'); either remove the unused route
from settings-router.tsx to eliminate dead code, or if it must be retained for
backwards compatibility, document its purpose in that router and update
getTabFromPath and handleTabChange to recognize and route to the 'edit' segment
(add 'edit' to the recognized tab list and ensure navigation generates
staff/{slug}/edit where appropriate), plus update any tests/comments
accordingly.
| const initialTab = getTabFromPath(route); | ||
| const [selectedTab, setSelectedTab] = useState<string>(initialTab); | ||
|
|
||
| const handleTabChange = (newTabId: string) => { | ||
| const urlSegment = newTabId === 'profile' ? '' : `/${newTabId}`; | ||
|
|
||
| updateRoute(`staff/${user.slug}${urlSegment}`); | ||
| setSelectedTab(newTabId); | ||
| }; |
There was a problem hiding this comment.
Browser back/forward navigation won't update the selected tab.
The selectedTab state is initialized from initialTab only on mount. When the route changes via browser navigation (back/forward), the component re-renders and initialTab is recomputed, but selectedTab retains its previous value, causing the UI to show the wrong tab.
Apply this fix to synchronize selectedTab with route changes:
const initialTab = getTabFromPath(route);
const [selectedTab, setSelectedTab] = useState<string>(initialTab);
+
+ // Sync selectedTab with route changes from browser navigation
+ useEffect(() => {
+ const tabFromRoute = getTabFromPath(route);
+ setSelectedTab(tabFromRoute);
+ }, [route]);
const handleTabChange = (newTabId: string) => {You'll need to add useEffect to the React import on line 4:
-import React, {useCallback, useState} from 'react';
+import React, {useCallback, useEffect, useState} from 'react';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/admin-x-settings/src/components/settings/general/user-detail-modal.tsx
around lines 415 to 423, selectedTab is only initialized from initialTab on
mount so browser back/forward route changes don't update the UI; add useEffect
(and import it from React on line 4) that watches initialTab (or route) and
calls setSelectedTab(initialTab) whenever it changes, ensuring the selectedTab
state stays synchronized with the current route; keep the existing
handleTabChange logic intact.
ref https://linear.app/ghost/issue/BER-3080/manage-preferences-link-in-member-emails-opens-profile-instead
/staff/:slug(profile),/staff/:slug/social-links,/staff/:slug/email-notifications