Browse Source

feat(bug reports): Optimistically update the feedback items mutation is finished (#58973)

When you're doing a any mutation we're going to optimistically update
any feedback items that are in the useApiQuery cache or the
useFetchFeedbackInfiniteListData cache. This means the list on the left,
and the visible item on the right.

Steps to see it in action:
1. Click to view the first feedback in the list to open it on the right
side
2. Click the checkboxes for the first 3 feedback items in the list
(including the one you have open)
3. Click "Mark all as read" or "unread" bulk operation

Notice that the visible feedback on the right side of the screen will
update immediately to reflect the new state.

Along the way I was able to re-combine the bulk-mutation helper with the
single-item-mutation helper, to save some repetition.

---

Hooks!

Here's a graph of all the hooks and how they depend on each other.
Consumers of feedback data (maybe new views, buttons, etc) should only
need to interface with either of the `useFetch*` hooks, and the
`useMutateFeedback` hook.

Hopefully `useFeedbackCache` will just work as we add new fields that
can be updated (like assigned to).
Also `useMutateFeedback` will need new tiny helpers to support things
like `assignTo`.
When we do "update all 100+ feedbacks" bulk operation, both these hooks
will probably need some changes again.
I setup the `FeedbackQueryKeys` context provider & hook. This guarantees
that queryKey objects are the exact same ref everywhere. So intead of
drilling the keys down through props, we can have the list-header, and
item header buttons all reference the same cached objects.

<img width="843" alt="SCR-20231031-jeum"
src="https://github.com/getsentry/sentry/assets/187460/16cfa993-b07b-4460-a15d-6a158e50f00d">


---

I also created some reusable code for mutations and put it inside
`queryClient.tsx` , `fetchMutation` is something that you can just pass
in like this, if you wanted, or wrap as i have (composition!):
```
const { ... } = useMutation({
  mutationFn: fetchMutation,
});
```
Ryan Albrecht 1 year ago
parent
commit
2fbc4627e8

+ 9 - 0
static/app/components/feedback/decodeFeedbackId.tsx

@@ -0,0 +1,9 @@
+import {decodeScalar} from 'sentry/utils/queryString';
+
+export default function decodeFeedbackId(val: string | string[] | null | undefined) {
+  const [, feedbackId] = decodeScalar(val, '').split(':');
+
+  // TypeScript thinks `feedbackId` is a string, but it could be undefined.
+  // See `noUncheckedIndexedAccess`
+  return feedbackId ?? '';
+}

+ 0 - 29
static/app/components/feedback/feedbackDataContext.tsx

@@ -1,29 +0,0 @@
-import {createContext, ReactNode, useContext} from 'react';
-
-import useFeedbackListQueryKey from 'sentry/components/feedback/useFeedbackListQueryKey';
-import useFetchFeedbackInfiniteListData, {
-  EMPTY_INFINITE_LIST_DATA,
-} from 'sentry/components/feedback/useFetchFeedbackInfiniteListData';
-import useOrganization from 'sentry/utils/useOrganization';
-
-interface Props {
-  children: ReactNode;
-}
-
-const FeedbackListDataContext = createContext<
-  ReturnType<typeof useFetchFeedbackInfiniteListData>
->(EMPTY_INFINITE_LIST_DATA);
-
-export function FeedbackDataContext({children}: Props) {
-  const organization = useOrganization();
-  const queryKey = useFeedbackListQueryKey({organization});
-  const contextValue = useFetchFeedbackInfiniteListData({queryKey});
-
-  return (
-    <FeedbackListDataContext.Provider value={contextValue}>
-      {children}
-    </FeedbackListDataContext.Provider>
-  );
-}
-
-export const useInfiniteFeedbackListData = () => useContext(FeedbackListDataContext);

+ 27 - 29
static/app/components/feedback/feedbackItem/feedbackItem.tsx

@@ -1,6 +1,11 @@
-import {Fragment, useEffect} from 'react';
+import {Fragment} from 'react';
 import styled from '@emotion/styled';
 
+import {
+  addErrorMessage,
+  addLoadingMessage,
+  addSuccessMessage,
+} from 'sentry/actionCreators/indicator';
 import Button from 'sentry/components/actions/button';
 import ProjectAvatar from 'sentry/components/avatar/projectAvatar';
 import {CopyToClipboardButton} from 'sentry/components/copyToClipboardButton';
@@ -18,48 +23,37 @@ import TextCopyInput from 'sentry/components/textCopyInput';
 import {IconLink} from 'sentry/icons';
 import {t} from 'sentry/locale';
 import {space} from 'sentry/styles/space';
-import {Event, GroupStatus} from 'sentry/types';
+import type {Event} from 'sentry/types';
+import {GroupStatus} from 'sentry/types';
 import type {FeedbackIssue} from 'sentry/utils/feedback/types';
-import useApi from 'sentry/utils/useApi';
 import useOrganization from 'sentry/utils/useOrganization';
 
 interface Props {
   eventData: Event | undefined;
   feedbackItem: FeedbackIssue;
-  refetchIssue: () => void;
   tags: Record<string, string>;
 }
 
-export default function FeedbackItem({
-  feedbackItem,
-  eventData,
-  refetchIssue,
-  tags,
-}: Props) {
+export default function FeedbackItem({feedbackItem, eventData, tags}: Props) {
   const organization = useOrganization();
   const hasReplayId = useFeedbackHasReplayId({feedbackId: feedbackItem.id});
   const {markAsRead, resolve} = useMutateFeedback({
-    feedbackId: feedbackItem.id,
+    feedbackIds: [feedbackItem.id],
     organization,
-    refetchIssue,
   });
-  const api = useApi();
-
-  const markReadUrl = `/organizations/${organization.slug}/issues/${feedbackItem.id}/`;
-
-  useEffect(() => {
-    (async () => {
-      await api.requestPromise(markReadUrl, {
-        method: 'PUT',
-        data: {hasSeen: true},
-      });
-      refetchIssue();
-    })();
-  }, []); // eslint-disable-line
 
   const url = eventData?.tags.find(tag => tag.key === 'url');
   const replayId = eventData?.contexts?.feedback?.replay_id;
 
+  const mutationOptions = {
+    onError: () => {
+      addErrorMessage(t('An error occurred while updating the feedback.'));
+    },
+    onSuccess: () => {
+      addSuccessMessage(t('Updated feedback'));
+    },
+  };
+
   return (
     <Fragment>
       <HeaderPanelItem>
@@ -93,9 +87,12 @@ export default function FeedbackItem({
             <ErrorBoundary mini>
               <Button
                 onClick={() => {
-                  feedbackItem.status === 'resolved'
-                    ? resolve(GroupStatus.UNRESOLVED)
-                    : resolve(GroupStatus.RESOLVED);
+                  addLoadingMessage(t('Updating feedback...'));
+                  const newStatus =
+                    feedbackItem.status === 'resolved'
+                      ? GroupStatus.UNRESOLVED
+                      : GroupStatus.RESOLVED;
+                  resolve(newStatus, mutationOptions);
                 }}
               >
                 {feedbackItem.status === 'resolved' ? t('Unresolve') : t('Resolve')}
@@ -104,7 +101,8 @@ export default function FeedbackItem({
             <ErrorBoundary mini>
               <Button
                 onClick={() => {
-                  feedbackItem.hasSeen ? markAsRead(false) : markAsRead(true);
+                  addLoadingMessage(t('Updating feedback...'));
+                  markAsRead(!feedbackItem.hasSeen, mutationOptions);
                 }}
               >
                 {feedbackItem.hasSeen ? t('Mark Unread') : t('Mark Read')}

+ 5 - 18
static/app/components/feedback/feedbackItem/feedbackItemLoader.tsx

@@ -1,22 +1,14 @@
 import FeedbackEmptyDetails from 'sentry/components/feedback/details/feedbackEmptyDetails';
 import FeedbackErrorDetails from 'sentry/components/feedback/details/feedbackErrorDetails';
 import FeedbackItem from 'sentry/components/feedback/feedbackItem/feedbackItem';
-import useFeedbackItemQueryKey from 'sentry/components/feedback/useFeedbackItemQueryKey';
+import useCurrentFeedbackId from 'sentry/components/feedback/useCurrentFeedbackId';
 import useFetchFeedbackData from 'sentry/components/feedback/useFetchFeedbackData';
 import Placeholder from 'sentry/components/placeholder';
 import {t} from 'sentry/locale';
-import useOrganization from 'sentry/utils/useOrganization';
 
 export default function FeedbackItemLoader() {
-  const organization = useOrganization();
-
-  const queryKeys = useFeedbackItemQueryKey({organization});
-  const {
-    issueResult,
-    issueData: feedbackIssue,
-    tags,
-    eventData: feedbackEvent,
-  } = useFetchFeedbackData(queryKeys);
+  const feedbackId = useCurrentFeedbackId();
+  const {issueResult, issueData, tags, eventData} = useFetchFeedbackData({feedbackId});
 
   // There is a case where we are done loading, but we're fetching updates
   // This happens when the user has seen a feedback, clicks around a bit, then
@@ -29,14 +21,9 @@ export default function FeedbackItemLoader() {
     <Placeholder height="100%" />
   ) : issueResult.isError ? (
     <FeedbackErrorDetails error={t('Unable to load feedback')} />
-  ) : !feedbackIssue ? (
+  ) : !issueData ? (
     <FeedbackEmptyDetails />
   ) : (
-    <FeedbackItem
-      eventData={feedbackEvent}
-      feedbackItem={feedbackIssue}
-      refetchIssue={issueResult.refetch}
-      tags={tags}
-    />
+    <FeedbackItem eventData={eventData} feedbackItem={issueData} tags={tags} />
   );
 }

+ 29 - 0
static/app/components/feedback/getFeedbackItemQueryKey.tsx

@@ -0,0 +1,29 @@
+import type {Organization} from 'sentry/types';
+import type {ApiQueryKey} from 'sentry/utils/queryClient';
+
+interface Props {
+  feedbackId: string;
+  organization: Organization;
+}
+
+export default function getFeedbackItemQueryKey({feedbackId, organization}: Props): {
+  eventQueryKey: ApiQueryKey | undefined;
+  issueQueryKey: ApiQueryKey | undefined;
+} {
+  return {
+    issueQueryKey: feedbackId
+      ? [
+          `/organizations/${organization.slug}/issues/${feedbackId}/`,
+          {
+            query: {
+              collapse: ['release', 'tags'],
+              expand: ['inbox', 'owners'],
+            },
+          },
+        ]
+      : undefined,
+    eventQueryKey: feedbackId
+      ? [`/organizations/${organization.slug}/issues/${feedbackId}/events/latest/`]
+      : undefined,
+  };
+}

+ 4 - 8
static/app/components/feedback/list/feedbackList.tsx

@@ -8,10 +8,10 @@ import {
 } from 'react-virtualized';
 import styled from '@emotion/styled';
 
-import {useInfiniteFeedbackListData} from 'sentry/components/feedback/feedbackDataContext';
 import FeedbackListHeader from 'sentry/components/feedback/list/feedbackListHeader';
 import FeedbackListItem from 'sentry/components/feedback/list/feedbackListItem';
 import useListItemCheckboxState from 'sentry/components/feedback/list/useListItemCheckboxState';
+import useFetchFeedbackInfiniteListData from 'sentry/components/feedback/useFetchFeedbackInfiniteListData';
 import LoadingIndicator from 'sentry/components/loadingIndicator';
 import PanelItem from 'sentry/components/panels/panelItem';
 import {Tooltip} from 'sentry/components/tooltip';
@@ -29,20 +29,16 @@ const cellMeasurer = {
 
 export default function FeedbackList() {
   const {
-    // error,
-    // hasNextPage,
-    // isError,
+    hasNextPage,
     isFetching, // If the network is active
     isFetchingNextPage,
     isFetchingPreviousPage,
     isLoading, // If anything is loaded yet
-    // Below are fields that are shims for react-virtualized
     getRow,
     isRowLoaded,
     issues,
     loadMoreRows,
-    // setFeedback,
-  } = useInfiniteFeedbackListData();
+  } = useFetchFeedbackInfiniteListData();
 
   const {setParamValue} = useUrlParams('query');
   const clearSearchTerm = () => setParamValue('');
@@ -96,7 +92,7 @@ export default function FeedbackList() {
         <InfiniteLoader
           isRowLoaded={isRowLoaded}
           loadMoreRows={loadMoreRows}
-          rowCount={issues.length}
+          rowCount={hasNextPage ? issues.length + 1 : issues.length}
         >
           {({onRowsRendered, registerChild}) => (
             <AutoSizer onResize={updateList}>

+ 31 - 10
static/app/components/feedback/list/feedbackListHeader.tsx

@@ -1,12 +1,17 @@
 import styled from '@emotion/styled';
 
+import {
+  addErrorMessage,
+  addLoadingMessage,
+  addSuccessMessage,
+} from 'sentry/actionCreators/indicator';
 import Button from 'sentry/components/actions/button';
 import Checkbox from 'sentry/components/checkbox';
 import {DropdownMenu} from 'sentry/components/dropdownMenu';
 import ErrorBoundary from 'sentry/components/errorBoundary';
 import decodeMailbox from 'sentry/components/feedback/decodeMailbox';
 import MailboxPicker from 'sentry/components/feedback/list/mailboxPicker';
-import useBulkMutateFeedback from 'sentry/components/feedback/useBulkMutateFeedback';
+import useMutateFeedback from 'sentry/components/feedback/useMutateFeedback';
 import PanelItem from 'sentry/components/panels/panelItem';
 import {Flex} from 'sentry/components/profiling/flex';
 import {IconEllipsis} from 'sentry/icons/iconEllipsis';
@@ -49,11 +54,20 @@ export default function FeedbackListHeader({checked, toggleChecked}: Props) {
 
 function HasSelection({checked, mailbox}) {
   const organization = useOrganization();
-  const {markAsRead, resolve} = useBulkMutateFeedback({
-    feedbackList: checked,
+  const {markAsRead, resolve} = useMutateFeedback({
+    feedbackIds: checked,
     organization,
   });
 
+  const mutationOptions = {
+    onError: () => {
+      addErrorMessage(t('An error occurred while updating the feedback.'));
+    },
+    onSuccess: () => {
+      addSuccessMessage(t('Updated feedback'));
+    },
+  };
+
   return (
     <Flex gap={space(1)} align="center" justify="space-between" style={{flexGrow: 1}}>
       <span>
@@ -62,11 +76,12 @@ function HasSelection({checked, mailbox}) {
       <Flex gap={space(1)} justify="flex-end">
         <ErrorBoundary mini>
           <Button
-            onClick={() =>
-              mailbox === 'resolved'
-                ? resolve(GroupStatus.UNRESOLVED)
-                : resolve(GroupStatus.RESOLVED)
-            }
+            onClick={() => {
+              addLoadingMessage(t('Updating feedback...'));
+              const newStatus =
+                mailbox === 'resolved' ? GroupStatus.UNRESOLVED : GroupStatus.RESOLVED;
+              resolve(newStatus, mutationOptions);
+            }}
           >
             {mailbox === 'resolved' ? t('Unresolve') : t('Resolve')}
           </Button>
@@ -84,12 +99,18 @@ function HasSelection({checked, mailbox}) {
               {
                 key: 'mark read',
                 label: t('Mark Read'),
-                onAction: () => markAsRead(true),
+                onAction: () => {
+                  addLoadingMessage(t('Updating feedback...'));
+                  markAsRead(true, mutationOptions);
+                },
               },
               {
                 key: 'mark unread',
                 label: t('Mark Unread'),
-                onAction: () => markAsRead(false),
+                onAction: () => {
+                  addLoadingMessage(t('Updating feedback...'));
+                  markAsRead(false, mutationOptions);
+                },
               },
             ]}
           />

+ 5 - 11
static/app/components/feedback/list/feedbackListItem.tsx

@@ -81,17 +81,11 @@ const FeedbackListItem = forwardRef<HTMLDivElement, Props>(
           <span style={{gridArea: 'time'}}>
             <TimeSince date={feedbackItem.firstSeen} />
           </span>
-          {feedbackItem.hasSeen ? null : (
-            <span
-              style={{
-                gridArea: 'unread',
-                display: 'flex',
-                justifyContent: 'center',
-              }}
-            >
-              <IconCircleFill size="xs" color="purple300" />
-            </span>
-          )}
+          <Flex justify="center" style={{gridArea: 'unread'}}>
+            {feedbackItem.hasSeen ? null : (
+              <IconCircleFill size="xs" color={isSelected ? 'white' : 'purple400'} />
+            )}
+          </Flex>
           <div style={{gridArea: 'message'}}>
             <TextOverflow>{feedbackItem.metadata.message}</TextOverflow>
           </div>

+ 0 - 78
static/app/components/feedback/useBulkMutateFeedback.tsx

@@ -1,78 +0,0 @@
-import {useCallback} from 'react';
-
-import {
-  addErrorMessage,
-  addLoadingMessage,
-  addSuccessMessage,
-} from 'sentry/actionCreators/indicator';
-import {t} from 'sentry/locale';
-import {GroupStatus, Organization} from 'sentry/types';
-import {useMutation} from 'sentry/utils/queryClient';
-import useApi from 'sentry/utils/useApi';
-
-interface Props {
-  feedbackList: string[];
-  organization: Organization;
-}
-
-type QueryKeyEndpointOptions = {
-  headers?: Record<string, string>;
-  query?: Record<string, any>;
-};
-type ApiMutationVariables =
-  | ['PUT' | 'POST' | 'DELETE', string]
-  | ['PUT' | 'POST' | 'DELETE', string, QueryKeyEndpointOptions]
-  | ['PUT' | 'POST', string, QueryKeyEndpointOptions, Record<string, unknown>];
-type TData = unknown;
-type TError = unknown;
-type TVariables = ApiMutationVariables;
-type TContext = unknown;
-
-export default function useBulkFeedbackItem({feedbackList, organization}: Props) {
-  const api = useApi();
-
-  const mutation = useMutation<TData, TError, TVariables, TContext>({
-    onMutate: (_variables: TVariables) => {
-      addLoadingMessage(t('Updating feedback...'));
-      // TODO: optimistic updates to the list cache, and the item cache with useFeedback*QueryKey() helpers
-    },
-    mutationFn: async (variables: ApiMutationVariables) => {
-      const [method, url, opts, data] = variables;
-      return await api.requestPromise(url, {
-        method,
-        query: opts?.query,
-        headers: opts?.headers,
-        data,
-      });
-    },
-    onError: () => {
-      addErrorMessage(t('An error occurred while updating the feedback.'));
-    },
-    onSuccess: () => {
-      addSuccessMessage(t('Updated feedback'));
-    },
-    onSettled: () => {},
-    cacheTime: 0,
-  });
-
-  const url = `/organizations/${organization.slug}/issues/`;
-
-  const markAsRead = useCallback(
-    (hasSeen: boolean) => {
-      mutation.mutate(['PUT', url, {query: {id: feedbackList}}, {hasSeen}]);
-    },
-    [mutation, url, feedbackList]
-  );
-
-  const resolve = useCallback(
-    (status: GroupStatus) => {
-      mutation.mutate(['PUT', url, {query: {id: feedbackList}}, {status}]);
-    },
-    [mutation, url, feedbackList]
-  );
-
-  return {
-    markAsRead,
-    resolve,
-  };
-}

+ 11 - 0
static/app/components/feedback/useCurrentFeedbackId.tsx

@@ -0,0 +1,11 @@
+import decodeFeedbackId from 'sentry/components/feedback/decodeFeedbackId';
+import useLocationQuery from 'sentry/utils/url/useLocationQuery';
+
+export default function useCurrentFeedbackId() {
+  const {feedbackSlug} = useLocationQuery({
+    fields: {
+      feedbackSlug: decodeFeedbackId,
+    },
+  });
+  return feedbackSlug;
+}

Some files were not shown because too many files changed in this diff