From 2bb244075cf9ba8c0996d4b46f452189811e1487 Mon Sep 17 00:00:00 2001 From: Robin Pyon Date: Thu, 19 Oct 2023 15:50:51 +0100 Subject: [PATCH] fix: ensure created comments create URLs with inspect and comment params (#5012) --- .../src/hooks/useCommentOperations.ts | 42 +++++++------ .../src/hooks/useNotificationTarget.ts | 62 +++++++++++-------- .../sanity/src/desk/comments/src/types.ts | 6 +- .../src/desk/components/paneRouter/types.ts | 2 +- 4 files changed, 65 insertions(+), 47 deletions(-) diff --git a/packages/sanity/src/desk/comments/src/hooks/useCommentOperations.ts b/packages/sanity/src/desk/comments/src/hooks/useCommentOperations.ts index a3f32446e48a..8b1380bc32d0 100644 --- a/packages/sanity/src/desk/comments/src/hooks/useCommentOperations.ts +++ b/packages/sanity/src/desk/comments/src/hooks/useCommentOperations.ts @@ -9,6 +9,8 @@ import { CommentPostPayload, } from '../types' import {useNotificationTarget} from './useNotificationTarget' +import {useWorkspace} from 'sanity' +import {useRouterState} from 'sanity/router' export interface CommentOperationsHookValue { operation: CommentOperations @@ -52,18 +54,28 @@ export function useCommentOperations( const authorId = currentUser?.id - const {documentTitle, toolName, url, workspaceTitle} = useNotificationTarget({ - documentId, - documentType, - }) + const activeToolName = useRouterState( + useCallback( + (routerState) => (typeof routerState.tool === 'string' ? routerState.tool : undefined), + [], + ), + ) + const {tools} = useWorkspace() + const activeTool = useMemo( + () => tools.find((tool) => tool.name === activeToolName), + [activeToolName, tools], + ) + const {getNotificationValue} = useNotificationTarget({documentId, documentType}) const handleCreate = useCallback( async (comment: CommentCreatePayload) => { + // The comment payload might already have an id if, for example, the comment was created + // but the request failed. In that case, we'll reuse the id when retrying to + // create the comment. + const commentId = comment?.id || uuid() + const nextComment = { - // The comment payload might already have an id if, for example, the comment was created - // but the request failed. In that case, we'll reuse the id when retrying to - // create the comment. - _id: comment?.id || uuid(), + _id: commentId, _type: 'comment', authorId: authorId || '', // improve lastEditedAt: undefined, @@ -76,12 +88,8 @@ export function useCommentOperations( payload: { workspace, }, - notification: { - documentTitle, - url, - workspaceTitle, - }, - tool: toolName, + notification: getNotificationValue({commentId}), + tool: activeTool?.name || '', }, target: { path: { @@ -114,19 +122,17 @@ export function useCommentOperations( } }, [ + activeTool?.name, authorId, client, dataset, documentId, - documentTitle, documentType, + getNotificationValue, onCreate, onCreateError, projectId, - toolName, - url, workspace, - workspaceTitle, ], ) diff --git a/packages/sanity/src/desk/comments/src/hooks/useNotificationTarget.ts b/packages/sanity/src/desk/comments/src/hooks/useNotificationTarget.ts index dc6386dcb088..4666fb98bcea 100644 --- a/packages/sanity/src/desk/comments/src/hooks/useNotificationTarget.ts +++ b/packages/sanity/src/desk/comments/src/hooks/useNotificationTarget.ts @@ -1,8 +1,10 @@ -import {useCallback, useMemo} from 'react' +import {useCallback} from 'react' import {useMemoObservable} from 'react-rx' import {of} from 'rxjs' -import {useRouterState} from 'sanity/router' -import {useSchema, useWorkspace, useDocumentPreviewStore, getPreviewStateObservable} from 'sanity' +import {usePaneRouter} from '../../../components' +import {COMMENTS_INSPECTOR_NAME} from '../../../panes/document/constants' +import {CommentContext} from '../types' +import {getPreviewStateObservable, useDocumentPreviewStore, useSchema, useWorkspace} from 'sanity' interface NotificationTargetHookOptions { documentId: string @@ -10,10 +12,16 @@ interface NotificationTargetHookOptions { } interface NotificationTargetHookValue { - documentTitle: string - toolName: string - url: string - workspaceTitle: string + /** + * Returns an object with notification-specific values for the selected comment, such as + * the current workspace + document title and full URL to the comment. + * These values are currently used in notification emails. + * + * **Please note:** this will generate a URL for the comment based on the current _active_ pane. + * The current active pane may not necessarily be the right-most desk pane and in these cases, + * the selected comment may not be visible on initial load when visiting these URLs. + */ + getNotificationValue: ({commentId}: {commentId: string}) => CommentContext['notification'] } /** @internal */ @@ -22,18 +30,8 @@ export function useNotificationTarget( ): NotificationTargetHookValue { const {documentId, documentType} = opts || {} const schemaType = useSchema().get(documentType) - const {basePath, title: workspaceTitle, tools} = useWorkspace() - - const activeToolName = useRouterState( - useCallback( - (routerState) => (typeof routerState.tool === 'string' ? routerState.tool : undefined), - [], - ), - ) - const activeTool = useMemo( - () => tools.find((tool) => tool.name === activeToolName), - [activeToolName, tools], - ) + const {title: workspaceTitle} = useWorkspace() + const {createPathWithParams, params} = usePaneRouter() const documentPreviewStore = useDocumentPreviewStore() @@ -45,15 +43,25 @@ export function useNotificationTarget( const {published, draft} = previewState || {} const documentTitle = (draft?.title || published?.title || 'Sanity document') as string - const currentUrl = new URL(window.location.href) - const deskToolSegment = currentUrl.pathname.split('/').slice(2, 3).join('') - currentUrl.pathname = `${basePath}/${deskToolSegment}/__edit__${documentId},type=${documentType}` - const notificationUrl = currentUrl.toString() + const handleGetNotificationValue = useCallback( + ({commentId}: {commentId: string}) => { + // Generate a path based on the current pane params. + // We force a value for `inspect` to ensure that this is included in URLs when comments + // are created outside of the inspector context (i.e. directly on the field) + // @todo: consider filtering pane router params and culling all non-active RHS panes prior to generating this link + const path = createPathWithParams({ + ...params, + comment: commentId, + inspect: COMMENTS_INSPECTOR_NAME, + }) + const url = `${window.location.origin}${path}` + + return {documentTitle, url, workspaceTitle} + }, + [createPathWithParams, documentTitle, params, workspaceTitle], + ) return { - documentTitle, - toolName: activeTool?.name || '', - url: notificationUrl, - workspaceTitle, + getNotificationValue: handleGetNotificationValue, } } diff --git a/packages/sanity/src/desk/comments/src/types.ts b/packages/sanity/src/desk/comments/src/types.ts index 8c4e7ac8a9ca..c9921013784a 100644 --- a/packages/sanity/src/desk/comments/src/types.ts +++ b/packages/sanity/src/desk/comments/src/types.ts @@ -69,7 +69,11 @@ export interface CommentPath { field: string } -interface CommentContext { +/** + * @beta + * @hidden + */ +export interface CommentContext { tool: string payload?: Record notification?: { diff --git a/packages/sanity/src/desk/components/paneRouter/types.ts b/packages/sanity/src/desk/components/paneRouter/types.ts index 708d27413d82..b111d0bf10dd 100644 --- a/packages/sanity/src/desk/components/paneRouter/types.ts +++ b/packages/sanity/src/desk/components/paneRouter/types.ts @@ -157,7 +157,7 @@ export interface PaneRouterContextValue { * A function that creates a path with the given parameters without navigating to it. * Useful for creating links that can be e.g. copied to clipboard and shared. */ - createPathWithParams: (params: Record) => void + createPathWithParams: (params: Record) => string /** * Proxied navigation to a given intent. Consider just exposing `router` instead?